From b4d812e69c800e574adbc9ad796c4a9df9b94f10 Mon Sep 17 00:00:00 2001 From: datadog-bits <263423550+datadog-bits@users.noreply.github.com> Date: Tue, 4 Aug 2026 22:49:25 +0000 Subject: [PATCH 1/3] Add reusable workflow diff --- docs/COMMANDS.md | 4 +- docs/EXAMPLES.md | 10 ++ src/commands/monitors.rs | 62 +++-------- src/commands/workflows.rs | 92 +++++++++++++++ src/main.rs | 25 +++++ src/util_ext.rs | 227 +++++++++++++++++++++++++++++++++++++- 6 files changed, 369 insertions(+), 51 deletions(-) diff --git a/docs/COMMANDS.md b/docs/COMMANDS.md index 217db7de..fe660c70 100644 --- a/docs/COMMANDS.md +++ b/docs/COMMANDS.md @@ -79,7 +79,7 @@ pup [options] # Nested commands | fleet | agents (list, get, versions, tracers), deployments (list, get, configure, upgrade, cancel), schedules (list, get, create, update, delete, trigger), tracers (list), clusters (list), instrumented-pods (list) | src/commands/fleet.rs | ✅ | | skills | list, install, path (positional ``: claude/cursor/codex/opencode/windsurf/gemini/pi/devin/all; `--name`, `--type`, `--project` for project-local scope) | src/commands/skills.rs | ✅ | | runbooks | list, describe, run, import, validate | src/commands/runbooks.rs | ✅ | -| workflows | get, create, update, delete, run, instances (list, get, cancel), connections (get, create, update, delete) | src/commands/workflows.rs | ✅ | +| workflows | get, create, update, diff, delete, run, instances (list, get, cancel), connections (get, create, update, delete) | src/commands/workflows.rs | ✅ | | investigations | list, get, trigger | src/commands/investigations.rs | ✅ | | change-requests | create, get, update, create-branch, decisions (update, delete) | src/commands/change_management.rs | ✅ | | change-stories | list | src/commands/change_stories.rs | ✅ | @@ -193,7 +193,7 @@ pup infrastructure hosts list - **hamr** - High Availability Multi-Region connections - **fleet** - Fleet Automation (agents, deployments, schedules, tracers, clusters, instrumented-pods) - **runbooks** - Local runbook execution engine (list, describe, run, import, validate) -- **workflows** - Workflow Automation (get, create, update, delete, run, instances, connections) +- **workflows** - Workflow Automation (get, create, update, diff, delete, run, instances, connections) - **investigations** - Bits AI SRE investigations (list, get, trigger) - **change-requests** - Change request management (create, get, update, create-branch, decisions) - **change-stories** - Change events for a service (deployments, feature flags, config, k8s, watchdog) over time window diff --git a/docs/EXAMPLES.md b/docs/EXAMPLES.md index c2c2a420..3e7a9cb8 100644 --- a/docs/EXAMPLES.md +++ b/docs/EXAMPLES.md @@ -773,6 +773,16 @@ pup workflows create --file=workflow.json pup workflows update --file=workflow.json ``` +### Diff a Workflow +```bash +# Compare a candidate JSON file against the live workflow +pup workflows diff workflow.json + +# Scope or suppress specific field paths +pup workflows diff workflow.json --only data.attributes.spec +pup workflows diff workflow.json --ignore data.attributes.updatedAt +``` + ### Delete a Workflow ```bash pup workflows delete diff --git a/src/commands/monitors.rs b/src/commands/monitors.rs index 0055f4d5..1d8994d6 100644 --- a/src/commands/monitors.rs +++ b/src/commands/monitors.rs @@ -114,7 +114,7 @@ pub async fn diff( // Trade-off: field-name typos in the candidate file won't be caught here; // they will appear as added/removed pairs in the diff output, which is still // actionable. `pup monitors update` will fail or no-op on unknown fields. - let mut candidate: serde_json::Value = util::read_json_file(file)?; + let candidate: serde_json::Value = util::read_json_file(file)?; // Fetch the live monitor let api = crate::make_api!(MonitorsAPI, cfg); @@ -122,57 +122,31 @@ pub async fn diff( .get_monitor(monitor_id, GetMonitorOptionalParams::default()) .await .map_err(|e| anyhow::anyhow!("failed to get monitor: {:?}", e))?; - let mut live = serde_json::to_value(&live) + let live = serde_json::to_value(&live) .map_err(|e| anyhow::anyhow!("failed to serialize monitor {monitor_id} for diff: {e:?}"))?; - // Normalize both sides: strip server-managed read-only fields and drop nulls - // so absent and explicit-null compare equal. - // Note: the API may populate concrete option defaults (e.g. new_host_delay, - // notify_no_data) in the live response that the candidate omits. Those appear - // as "removed" because the candidate is treated as the complete desired state. - // Use --ignore to suppress specific option fields if the noise is unwanted. - util_ext::normalize_for_diff(&mut live, util_ext::READONLY_MONITOR_FIELDS); - util_ext::normalize_for_diff(&mut candidate, util_ext::READONLY_MONITOR_FIELDS); - - let entries = util_ext::scope_diff(util_ext::diff_json(&live, &candidate), only, ignore); - // `update` (PUT) is a partial/merge update: fields absent from the candidate // file are left unchanged on the live monitor, not deleted. "removed" entries // in this diff show what the candidate does not specify — they will NOT be // removed by `pup monitors update`. Run `update` only for "added"/"modified" // changes; "removed" entries require no action unless you want to add those // fields to the candidate explicitly. - let has_removed = entries - .iter() - .any(|e| e.change == util_ext::ChangeKind::Removed); - let next_action = if entries.is_empty() { - None - } else if has_removed { - Some( - "review changes — note: 'removed' entries will NOT be deleted by \ - `pup monitors update` (partial update)" - .to_string(), - ) - } else { - Some("review changes, then run `pup monitors update`".to_string()) - }; - let meta = Metadata { - count: Some(entries.len()), - truncated: false, - command: Some("monitors diff".to_string()), - next_action, - }; - formatter::format_and_print( - &entries, - &cfg.output_format, - cfg.agent_mode, - Some(&meta), - cfg.jq.as_deref(), - )?; - if entries.is_empty() && !cfg.agent_mode { - eprintln!("No changes — monitor {monitor_id} is in sync."); - } - Ok(()) + let resource_id = monitor_id.to_string(); + let mut options = util_ext::ResourceDiffOptions::new( + "monitors diff", + "pup monitors update", + "monitor", + &resource_id, + ); + options.readonly_paths = util_ext::READONLY_MONITOR_FIELDS; + options.only = only; + options.ignore = ignore; + options.removed_entries_next_action = Some( + "review changes — note: 'removed' entries will NOT be deleted by \ + `pup monitors update` (partial update)", + ); + options.no_changes_message = Some(format!("No changes — monitor {monitor_id} is in sync.")); + util_ext::format_resource_diff(cfg, &live, &candidate, &options) } pub async fn search( diff --git a/src/commands/workflows.rs b/src/commands/workflows.rs index 5faf8aa8..573a4f17 100644 --- a/src/commands/workflows.rs +++ b/src/commands/workflows.rs @@ -54,6 +54,36 @@ pub async fn update(cfg: &Config, workflow_id: &str, file: &str) -> Result<()> { formatter::output(cfg, &resp) } +pub async fn diff( + cfg: &Config, + workflow_id: &str, + file: &str, + only: &[String], + ignore: &[String], +) -> Result<()> { + let candidate: serde_json::Value = util::read_json_file(file)?; + let api = make_api(cfg); + let live = api + .get_workflow(workflow_id.to_string()) + .await + .map_err(|e| anyhow::anyhow!("failed to get workflow: {:?}", e))?; + let live = serde_json::to_value(&live).map_err(|e| { + anyhow::anyhow!("failed to serialize workflow {workflow_id} for diff: {e:?}") + })?; + + let mut options = util_ext::ResourceDiffOptions::new( + "workflows diff", + "pup workflows update", + "workflow", + workflow_id, + ); + options.readonly_paths = util_ext::READONLY_WORKFLOW_FIELDS; + options.only = only; + options.ignore = ignore; + options.no_changes_message = Some(format!("No changes - workflow {workflow_id} is in sync.")); + util_ext::format_resource_diff(cfg, &live, &candidate, &options) +} + pub async fn delete(cfg: &Config, workflow_id: &str) -> Result<()> { let api = make_api(cfg); api.delete_workflow(workflow_id.to_string()) @@ -270,6 +300,68 @@ mod tests { use crate::test_support::*; + #[tokio::test] + async fn test_workflows_diff_detects_changes() { + let _lock = lock_env().await; + let mut server = mockito::Server::new_async().await; + let cfg = test_config(&server.url()); + + let live_body = r#"{ + "data": { + "id": "wf-123", + "type": "workflows", + "attributes": { + "name": "Old workflow", + "description": "Deploy service", + "spec": {"steps": [{"name": "deploy", "timeout": 60}]}, + "updatedAt": "2024-01-01T00:00:00Z" + } + } + }"#; + let _mock = mock_any(&mut server, "GET", live_body).await; + + let candidate = r#"{ + "data": { + "type": "workflows", + "attributes": { + "name": "New workflow", + "description": "Deploy service", + "spec": {"steps": [{"name": "deploy", "timeout": 90}]} + } + } + }"#; + let path = write_temp_json("pup_workflows_diff_detects_changes.json", candidate); + + let result = super::diff(&cfg, "wf-123", path.to_str().unwrap(), &[], &[]).await; + let _ = std::fs::remove_file(path); + assert!(result.is_ok(), "workflows diff failed: {:?}", result.err()); + cleanup_env(); + } + + #[tokio::test] + async fn test_workflows_diff_file_not_found() { + let cfg = test_config("http://unused.local"); + let result = super::diff(&cfg, "wf-123", "/nonexistent/path.json", &[], &[]).await; + assert!(result.is_err()); + assert!(result + .unwrap_err() + .to_string() + .contains("failed to read file")); + } + + #[tokio::test] + async fn test_workflows_diff_invalid_json() { + let path = write_temp_json("pup_workflows_diff_invalid_json.json", "not valid json {{{"); + let cfg = test_config("http://unused.local"); + let result = super::diff(&cfg, "wf-123", path.to_str().unwrap(), &[], &[]).await; + let _ = std::fs::remove_file(path); + assert!(result.is_err()); + assert!(result + .unwrap_err() + .to_string() + .contains("failed to parse JSON")); + } + #[tokio::test] async fn test_connections_get() { let _lock = lock_env().await; diff --git a/src/main.rs b/src/main.rs index b429f8c6..0ca581dc 100644 --- a/src/main.rs +++ b/src/main.rs @@ -4744,6 +4744,23 @@ enum WorkflowActions { #[arg(long)] file: String, }, + /// Diff a candidate JSON definition against the live workflow + Diff { + workflow_id: String, + file: String, + #[arg( + long, + value_delimiter = ',', + help = "Restrict the diff to these field paths (dot-notation, comma-separated or repeated)" + )] + only: Vec, + #[arg( + long, + value_delimiter = ',', + help = "Exclude these field paths from the diff (dot-notation, comma-separated or repeated)" + )] + ignore: Vec, + }, /// Delete a workflow Delete { workflow_id: String }, /// Execute a workflow via API trigger @@ -16523,6 +16540,14 @@ async fn main_inner() -> anyhow::Result<()> { WorkflowActions::Update { workflow_id, file } => { commands::workflows::update(&cfg, &workflow_id, &file).await?; } + WorkflowActions::Diff { + workflow_id, + file, + only, + ignore, + } => { + commands::workflows::diff(&cfg, &workflow_id, &file, &only, &ignore).await?; + } WorkflowActions::Delete { workflow_id } => { commands::workflows::delete(&cfg, &workflow_id).await?; } diff --git a/src/util_ext.rs b/src/util_ext.rs index c5ef162c..0cf9bde2 100644 --- a/src/util_ext.rs +++ b/src/util_ext.rs @@ -1,4 +1,4 @@ -//! Hand-written pup command utilities: time/duration parsing, the monitor-diff +//! Hand-written pup command utilities: time/duration parsing, the JSON diff //! helpers, compute-string parsing, and percent-encoding. None of this is used by //! generated command modules (only `util::read_json_file` is) -- kept here as pup- //! owned code the generator never touches. @@ -11,6 +11,8 @@ use regex::Regex; use serde::Serialize; use serde_json::Value; +use crate::formatter::{self, Metadata}; + fn parse_relative_duration_millis(input: &str) -> Result { let stripped = input.trim_start_matches('-').trim(); @@ -185,24 +187,184 @@ pub struct DiffEntry { pub after: Option, } -/// Remove `readonly` keys at the top level and recursively drop `null`-valued +/// Read-only server-managed fields that are stripped before diffing a workflow. +pub const READONLY_WORKFLOW_FIELDS: &[&str] = &[ + "data.id", + "data.type", + "data.attributes.created_at", + "data.attributes.createdAt", + "data.attributes.created_by", + "data.attributes.createdBy", + "data.attributes.modified_at", + "data.attributes.modifiedAt", + "data.attributes.updated_at", + "data.attributes.updatedAt", + "data.attributes.updated_by", + "data.attributes.updatedBy", +]; + +/// Options for diffing a live resource against a candidate JSON value. +pub struct ResourceDiffOptions<'a> { + pub command: &'a str, + pub update_command: &'a str, + pub resource_kind: &'a str, + pub resource_id: &'a str, + pub readonly_paths: &'a [&'a str], + pub only: &'a [String], + pub ignore: &'a [String], + pub live_root: Option<&'a str>, + pub candidate_root: Option<&'a str>, + pub removed_entries_next_action: Option<&'a str>, + pub no_changes_message: Option, +} + +impl<'a> ResourceDiffOptions<'a> { + pub fn new( + command: &'a str, + update_command: &'a str, + resource_kind: &'a str, + resource_id: &'a str, + ) -> Self { + Self { + command, + update_command, + resource_kind, + resource_id, + readonly_paths: &[], + only: &[], + ignore: &[], + live_root: None, + candidate_root: None, + removed_entries_next_action: None, + no_changes_message: None, + } + } +} + +/// Remove `readonly` keys at dotted paths and recursively drop `null`-valued /// keys so that absent fields and explicit `null`s compare equal. pub fn normalize_for_diff(v: &mut Value, readonly: &[&str]) { + for key in readonly { + remove_path(v, key); + } + drop_null_object_fields(v); +} + +fn remove_path(v: &mut Value, path: &str) { + let parts: Vec<&str> = path.split('.').filter(|part| !part.is_empty()).collect(); + remove_path_parts(v, &parts); +} + +fn remove_path_parts(v: &mut Value, parts: &[&str]) { + if parts.is_empty() { + return; + } + if let Value::Object(map) = v { - for key in readonly { - map.remove(*key); + if parts.len() == 1 { + map.remove(parts[0]); + } else if let Some(child) = map.get_mut(parts[0]) { + remove_path_parts(child, &parts[1..]); } + } +} + +fn drop_null_object_fields(v: &mut Value) { + if let Value::Object(map) = v { let keys: Vec = map.keys().cloned().collect(); for key in keys { if map[&key].is_null() { map.remove(&key); } else { - normalize_for_diff(map.get_mut(&key).unwrap(), &[]); + drop_null_object_fields(map.get_mut(&key).unwrap()); } } } } +/// Diff a live resource and candidate using common pup resource-diff rules. +pub fn diff_resource_values( + live: &Value, + candidate: &Value, + options: &ResourceDiffOptions<'_>, +) -> Result> { + let mut live = select_diff_root(live, options.live_root, "live")?; + let mut candidate = select_diff_root(candidate, options.candidate_root, "candidate")?; + normalize_for_diff(&mut live, options.readonly_paths); + normalize_for_diff(&mut candidate, options.readonly_paths); + Ok(scope_diff( + diff_json(&live, &candidate), + options.only, + options.ignore, + )) +} + +fn select_diff_root(value: &Value, root: Option<&str>, label: &str) -> Result { + let Some(path) = root else { + return Ok(value.clone()); + }; + + let mut current = value; + for part in path.split('.').filter(|part| !part.is_empty()) { + current = current + .get(part) + .ok_or_else(|| anyhow::anyhow!("{label} JSON does not contain diff root '{path}'"))?; + } + Ok(current.clone()) +} + +/// Diff and print a live resource against a candidate JSON value. +pub fn format_resource_diff( + cfg: &crate::config::Config, + live: &Value, + candidate: &Value, + options: &ResourceDiffOptions<'_>, +) -> Result<()> { + let entries = diff_resource_values(live, candidate, options)?; + let has_removed = entries.iter().any(|e| e.change == ChangeKind::Removed); + let next_action = if entries.is_empty() { + None + } else if has_removed { + options + .removed_entries_next_action + .map(ToString::to_string) + .or_else(|| { + Some(format!( + "review changes, then run `{}`", + options.update_command + )) + }) + } else { + Some(format!( + "review changes, then run `{}`", + options.update_command + )) + }; + let meta = Metadata { + count: Some(entries.len()), + truncated: false, + command: Some(options.command.to_string()), + next_action, + }; + formatter::format_and_print( + &entries, + &cfg.output_format, + cfg.agent_mode, + Some(&meta), + cfg.jq.as_deref(), + )?; + if entries.is_empty() && !cfg.agent_mode { + let message = options.no_changes_message.clone().unwrap_or_else(|| { + format!( + "No changes - {} {} is in sync.", + options.resource_kind, options.resource_id + ) + }); + eprintln!("{message}"); + } + Ok(()) +} + /// Recursively compare `before` and `after` as `serde_json::Value`s, building /// dot-notation change records. Objects recurse; scalars and arrays compare as /// whole values. Returns entries sorted by path for deterministic output. @@ -789,6 +951,28 @@ mod tests { assert!(obj.contains_key("name")); } + #[test] + fn test_normalize_strips_nested_readonly_fields() { + let mut v = serde_json::json!({ + "data": { + "id": "wf-1", + "type": "workflows", + "attributes": { + "name": "Deploy", + "updatedAt": "2024-01-02T00:00:00Z" + } + } + }); + normalize_for_diff(&mut v, READONLY_WORKFLOW_FIELDS); + assert!(v.pointer("/data/id").is_none()); + assert!(v.pointer("/data/type").is_none()); + assert!(v.pointer("/data/attributes/updatedAt").is_none()); + assert_eq!( + v.pointer("/data/attributes/name"), + Some(&serde_json::json!("Deploy")) + ); + } + #[test] fn test_normalize_drops_null_values() { let mut v = serde_json::json!({"name": "cpu", "message": null, "priority": null}); @@ -809,6 +993,39 @@ mod tests { assert!(diff_json(&live, &candidate).is_empty()); } + #[test] + fn test_diff_resource_values_uses_roots_and_filters() { + let live = serde_json::json!({ + "data": { + "attributes": { + "name": "Old", + "spec": {"threshold": 1}, + "updatedAt": "2024-01-01T00:00:00Z" + } + } + }); + let candidate = serde_json::json!({ + "data": { + "attributes": { + "name": "New", + "spec": {"threshold": 2} + } + } + }); + let only = vec!["spec".to_string()]; + let readonly = ["updatedAt"]; + let mut options = ResourceDiffOptions::new("test diff", "test update", "thing", "id"); + options.readonly_paths = &readonly; + options.only = &only; + options.live_root = Some("data.attributes"); + options.candidate_root = Some("data.attributes"); + + let entries = diff_resource_values(&live, &candidate, &options).unwrap(); + assert_eq!(entries.len(), 1); + assert_eq!(entries[0].path, "spec.threshold"); + assert_eq!(entries[0].change, ChangeKind::Modified); + } + // ---- scope_diff ---- // Shorthand for building a Modified DiffEntry in scope_diff tests. From 97051e27b903c219e8e6c8ecdbf5cf9ef0850026 Mon Sep 17 00:00:00 2001 From: "datadog-datadog-prod-us1[bot]" <88084959+datadog-datadog-prod-us1[bot]@users.noreply.github.com> Date: Thu, 6 Aug 2026 14:18:00 +0000 Subject: [PATCH 2/3] Fix workflow diff test fixture Co-authored-by: platinummonkey --- src/commands/workflows.rs | 1 + src/util_ext.rs | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/src/commands/workflows.rs b/src/commands/workflows.rs index 573a4f17..914f4115 100644 --- a/src/commands/workflows.rs +++ b/src/commands/workflows.rs @@ -311,6 +311,7 @@ mod tests { "id": "wf-123", "type": "workflows", "attributes": { + "action_id": "wf-123", "name": "Old workflow", "description": "Deploy service", "spec": {"steps": [{"name": "deploy", "timeout": 60}]}, diff --git a/src/util_ext.rs b/src/util_ext.rs index 0cf9bde2..24036157 100644 --- a/src/util_ext.rs +++ b/src/util_ext.rs @@ -191,6 +191,8 @@ pub struct DiffEntry { pub const READONLY_WORKFLOW_FIELDS: &[&str] = &[ "data.id", "data.type", + "data.attributes.action_id", + "data.attributes.actionId", "data.attributes.created_at", "data.attributes.createdAt", "data.attributes.created_by", @@ -958,6 +960,7 @@ mod tests { "id": "wf-1", "type": "workflows", "attributes": { + "action_id": "wf-1", "name": "Deploy", "updatedAt": "2024-01-02T00:00:00Z" } @@ -966,6 +969,7 @@ mod tests { normalize_for_diff(&mut v, READONLY_WORKFLOW_FIELDS); assert!(v.pointer("/data/id").is_none()); assert!(v.pointer("/data/type").is_none()); + assert!(v.pointer("/data/attributes/action_id").is_none()); assert!(v.pointer("/data/attributes/updatedAt").is_none()); assert_eq!( v.pointer("/data/attributes/name"), From 0b41c030267af67c08ea272e902f4027d5529d9a Mon Sep 17 00:00:00 2001 From: "datadog-datadog-prod-us1[bot]" <88084959+datadog-datadog-prod-us1[bot]@users.noreply.github.com> Date: Thu, 6 Aug 2026 14:43:32 +0000 Subject: [PATCH 3/3] Fetch workflow diff JSON raw --- src/commands/workflows.rs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/commands/workflows.rs b/src/commands/workflows.rs index 914f4115..680871d6 100644 --- a/src/commands/workflows.rs +++ b/src/commands/workflows.rs @@ -8,6 +8,7 @@ use datadog_api_client::datadogV2::api_workflow_automation::{ use crate::config::Config; use crate::formatter::{self, Metadata}; +use crate::raw_client; use crate::util; use crate::util_ext; @@ -62,14 +63,9 @@ pub async fn diff( ignore: &[String], ) -> Result<()> { let candidate: serde_json::Value = util::read_json_file(file)?; - let api = make_api(cfg); - let live = api - .get_workflow(workflow_id.to_string()) + let live = raw_client::raw_get(cfg, &format!("/api/v2/workflows/{workflow_id}"), &[]) .await - .map_err(|e| anyhow::anyhow!("failed to get workflow: {:?}", e))?; - let live = serde_json::to_value(&live).map_err(|e| { - anyhow::anyhow!("failed to serialize workflow {workflow_id} for diff: {e:?}") - })?; + .map_err(|e| anyhow::anyhow!("failed to get workflow: {e:?}"))?; let mut options = util_ext::ResourceDiffOptions::new( "workflows diff",