diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 31de98c99..0949899f2 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -2,6 +2,7 @@ cmd/workload/ @datarobot-oss/workload-cli cmd/artifact/ @datarobot-oss/workload-cli internal/workload/ @datarobot-oss/workload-cli +internal/uidiff/ @datarobot-oss/workload-cli cmd/pipeline/ @datarobot-oss/compute-services internal/pipeline/ @datarobot-oss/compute-services docs/commands/pipeline*.md @datarobot-oss/compute-services diff --git a/CHANGELOG.md b/CHANGELOG.md index 226f9137f..7f14ff7a5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - `dr workload status`, `dr workload get`, `dr workload endpoint` and `dr workload logs` now take the workload id as an optional argument. Left out, it is read from the `workloadId` in the nearest `.datarobot.yaml`, searched upward from the new `--dir` flag (the current directory by default), so the commands `dr workload up` points you at can be run as printed from the project it just deployed. A typed id still wins, and is used without reading any manifest. The workload that was picked is named on stderr, except under `--output-format json`, where anything on stderr would break `2>&1 | jq .`. - `dr workload stop`, `dr workload start` and `dr workload delete` take the workload id optionally too, on the same terms. Because they change something, a workload named by the manifest rather than by you is confirmed first; a typed id is never questioned. `--yes` skips the question, and `stop` and `start` gained that flag for this. `DATAROBOT_CLI_NON_INTERACTIVE=1` also skips it on `stop` and `start`, but not on a manifest-named `delete`: that variable is set once across a pipeline, and deleting something nobody named is not what it was set for. - `dr workload delete --dir `: which project's manifest holds the binding to clear, matching the flag `up` and `config` already take. +- `dr workload up --diff` and `dr workload up --confirm`: the plan can now be reviewed as a unified diff, with context around each change and nothing truncated, and a run asked with `--confirm` deploys only after a `y` to `? Apply this deploy? (y/N)` on stderr, declining having changed nothing. `--dry-run --diff` is the look-without-touching combination, and when the run is non-interactive (`--yes`, `--output-format json`, `DATAROBOT_CLI_NON_INTERACTIVE`, or stdin that is not a terminal) `--confirm` is suppressed and behaves as if it was not given, because a question nobody can answer would hang the run instead of keeping it safe. ## Fixed diff --git a/cmd/workload/up/cmd.go b/cmd/workload/up/cmd.go index 135793a72..df1f3cad6 100644 --- a/cmd/workload/up/cmd.go +++ b/cmd/workload/up/cmd.go @@ -82,12 +82,14 @@ func buildID(id string) *string { } type flags struct { - dir string - yes bool - dryRun bool - detach bool - lock bool - force bool + dir string + yes bool + dryRun bool + diff bool + confirm bool + detach bool + lock bool + force bool // bindingFlags exist only to be refused. Cobra's own "unknown flag" // message would leave the user guessing where binding lives, and these @@ -148,6 +150,13 @@ allocation, is applied in place instead. Nothing is built and no version is made, because what the workload runs has not changed. A deploy that moves both sends the sizing with the rollout, so the new version comes up with it. +Two flags are for looking before leaping. --diff prints the plan as a unified +diff, with context around each change and nothing truncated, instead of the +changed-fields summary. --confirm asks '? Apply this deploy? (y/N)' on stderr +after the plan is printed and deploys only on yes; when the run is +non-interactive it is suppressed and behaves as if it was not given. To look +without touching, --dry-run --diff is the combination. + A workload that is not ready to be deployed onto is dealt with rather than refused. One still starting or stopping is waited out and then re-read, so the plan is built against where it landed. A stopped one is started and then @@ -179,6 +188,8 @@ Examples: return map[string]any{ "yes": nonInteractive, "dry_run": f.dryRun, + "diff": f.diff, + "confirm": f.confirm, "detach": f.detach, "lock": f.lock, "force_build": f.force, @@ -195,6 +206,22 @@ func addFlags(cmd *cobra.Command, f *flags, poll *pollflags.Set) { "Do not prompt. With no manifest this is an error rather than a wizard, "+ "and rolling a locked production version is not confirmed.") cmd.Flags().BoolVar(&f.dryRun, "dry-run", false, "Print the plan and change nothing.") + // The help says what changes about the rendering rather than leaving the + // reader to run it to find out: the default plan is the summary, the + // diff is the detail, and neither says anything the other does not. + cmd.Flags().BoolVar(&f.diff, "diff", false, + "Render the plan as a unified diff instead of the changed-fields list: "+ + "every field the file names appears, unchanged ones as context that collapses when it runs long, "+ + "and nothing is truncated. Combine with --dry-run to look without touching.") + // The suppression is documented in the flag itself because it is the one + // surprise the flag carries: someone piping output and passing --confirm + // would otherwise wait on a prompt that never comes, with no word of why. + cmd.Flags().BoolVar(&f.confirm, "confirm", false, + "Ask '? Apply this deploy? (y/N)' on stderr after printing the plan, and deploy only on yes; "+ + "anything else, including an empty answer, declines and changes nothing. "+ + "Suppressed when the run is non-interactive: --yes, --output-format json, "+ + "DATAROBOT_CLI_NON_INTERACTIVE, or a stdin that is not a terminal. "+ + "When suppressed it behaves as if the flag was not given.") cmd.Flags().BoolVar(&f.detach, "detach", false, "Return once the deploy is requested; do not wait for it to serve.") cmd.Flags().BoolVar(&f.lock, "lock", false, "Lock whichever artifact ends up live, making it permanent, even when this deploy minted no new "+ @@ -239,9 +266,11 @@ func run(cmd *cobra.Command, f flags, poll pollflags.Set, format outputformat.Ou Dir: dir, NonInteractive: nonInteractive, DryRun: f.dryRun, + Diff: f.diff, Detach: f.detach, Lock: f.lock, Confirm: rollConfirm(cmd, yes), + ConfirmApply: applyConfirm(cmd, f.confirm, nonInteractive), ForceBuild: f.force, PollInterval: poll.Interval, PollTimeout: poll.Timeout, @@ -249,6 +278,15 @@ func run(cmd *cobra.Command, f flags, poll pollflags.Set, format outputformat.Ou Spinner: !json && !nonInteractive, }) + if errors.Is(runErr, up.ErrDeclined) { + // The gate said no. Nothing has been touched, so there is no endpoint + // to print and no envelope to emit: the refusal itself is the whole + // outcome, and it reaches stderr through the same path every error + // takes. Falling through to render here would print the endpoint of a + // workload this run deliberately left alone. + return runErr + } + if runErr != nil && !reportable(result) { return runErr } @@ -315,6 +353,43 @@ func rollConfirm(cmd *cobra.Command, yes bool) func(question, want string) (bool return typedConfirm(cmd) } +// applyConfirm is the opt-in y/N gate behind --confirm, and nil when there is +// nobody to answer it. Suppressed, not refused: a run that cannot be asked +// behaves as if the flag was not given, which is the convention every prompt +// on this command follows. --yes is already the answer, -o json and a piped +// stdin mean nobody is reading, and DATAROBOT_CLI_NON_INTERACTIVE says the +// same; refusing the combination would break a scripted caller to protect a +// default that suppression already keeps safe. There is deliberately no cobra +// mutual exclusivity with --yes for the same reason. +// +// The question goes to stderr and the answer comes from stdin, like the typed +// confirm below: stdout is the endpoint, or one JSON document, and a question +// printed into it would break whatever is parsing it. +func applyConfirm(cmd *cobra.Command, confirm, nonInteractive bool) func(string) (bool, error) { + if !confirm || nonInteractive { + return nil + } + + return func(question string) (bool, error) { + fmt.Fprintf(cmd.ErrOrStderr(), "? %s (y/N) ", question) + + scanner := bufio.NewScanner(cmd.InOrStdin()) + if !scanner.Scan() { + if err := scanner.Err(); err != nil { + return false, fmt.Errorf("cannot read the answer: %w", err) + } + + return false, nil + } + + // The default is no, so an enter pressed twice in a row, a stray + // keystroke and a closed pipe can never deploy on their own. + answer := strings.ToLower(strings.TrimSpace(scanner.Text())) + + return answer == "y" || answer == "yes", nil + } +} + // typedConfirm asks a question that only the exact expected word answers. // // It goes to stderr and reads a single line, like everything else the deploy @@ -399,6 +474,18 @@ func draftIsServing(f flags, result up.Result, failed bool) bool { return f.dryRun || result.WorkloadID != "" } +// planEnvelope picks the plan's machine shape. --diff adds the structured +// diff section to the same document; without the flag the envelope keeps the +// exact shape it had before the flag existed, so a consumer that never asked +// for a diff never has to learn about one. +func planEnvelope(p up.Plan, diff bool) up.PlanJSON { + if diff { + return p.JSONWithDiff() + } + + return p.JSON() +} + func render(cmd *cobra.Command, f flags, format outputformat.OutputFormat, result up.Result, failed bool) error { if format == outputformat.OutputFormatJSON { return outputformat.PrintJSONEnvelope(cmd.OutOrStdout(), "up", upResult{ @@ -410,7 +497,7 @@ func render(cmd *cobra.Command, f flags, format outputformat.OutputFormat, resul BuildID: buildID(result.BuildID), Action: result.Action, Locked: result.Locked, - Plan: result.Plan.JSON(), + Plan: planEnvelope(result.Plan, f.diff), }) } diff --git a/cmd/workload/up/cmd_test.go b/cmd/workload/up/cmd_test.go index 00057358d..5606fba0f 100644 --- a/cmd/workload/up/cmd_test.go +++ b/cmd/workload/up/cmd_test.go @@ -23,6 +23,7 @@ import ( "strings" "testing" + "github.com/datarobot/cli/internal/telemetry" "github.com/datarobot/cli/internal/workload/manifest" "github.com/datarobot/cli/internal/workload/up" "github.com/stretchr/testify/assert" @@ -612,6 +613,138 @@ func TestCmd_UnchangedRunStillWarnsAboutTheDraft(t *testing.T) { assert.Contains(t, stderr, draftHeadline) } +// TestCmd_DiffIsRegistered keeps the flag discoverable: cobra lists it in +// --help only when it is registered, not hidden, and defaulted off, and the +// help text is what tells a reader what they are about to get. +func TestCmd_DiffIsRegistered(t *testing.T) { + lookup := Cmd().Flags().Lookup("diff") + + require.NotNil(t, lookup, "the flag has to exist for --diff to parse at all") + assert.False(t, lookup.Hidden) + assert.Equal(t, "false", lookup.DefValue, "the diff rendering is opt-in") + assert.Contains(t, lookup.Usage, "unified diff") +} + +// TestCmd_DiffReachesTheDeploy threads the flag into the deploy's options, +// and only when it was given: a run without it must not silently start +// rendering diffs. +func TestCmd_DiffReachesTheDeploy(t *testing.T) { + seen := stubRun(t, deployed(), nil) + + _, _, err := runCmd(t, "--dry-run", "--diff") + require.NoError(t, err) + assert.True(t, seen.Diff) + assert.True(t, seen.DryRun) + + defaults := stubRun(t, deployed(), nil) + + _, _, err = runCmd(t) + require.NoError(t, err) + assert.False(t, defaults.Diff) +} + +// TestCmd_TelemetryRecordsTheDiffFlag: adoption of the flag has to be +// readable without guessing at it from dry_run, so it is reported under its +// own key, reflecting the flag as given. +func TestCmd_TelemetryRecordsTheDiffFlag(t *testing.T) { + withFlag := Cmd() + require.NoError(t, withFlag.ParseFlags([]string{"--diff"})) + + event, ok := telemetry.EventFor(withFlag, nil) + require.True(t, ok, "EventFor must return ok=true for an annotated command") + + assert.Equal(t, true, event.EventProperties["diff"]) + assert.Equal(t, false, event.EventProperties["dry_run"], "the pre-existing keys keep their own values") + + event, ok = telemetry.EventFor(Cmd(), nil) + require.True(t, ok) + assert.Equal(t, false, event.EventProperties["diff"], "an unset flag reports itself as off, not as absent") +} + +// diffedResult is a result whose plan carries one change in each half, a +// whole row structure and an unmanaged path, the shape the JSON diff +// envelope tests read through the stubbed deploy. +func diffedResult() up.Result { + return up.Result{ + Plan: up.Plan{ + State: up.StateRunning, + Code: up.CodeChange{}, + Artifact: []up.Change{{Path: "containerGroups[default].containers[primary].port", Have: 8080.0, Want: 9090.0}}, + Runtime: []up.Change{{Path: "containerGroups[default].replicaCount", Have: 1.0, Want: 3.0}}, + + DiffArtifact: []up.DiffRow{{ + Path: "containerGroups[default].containers[primary].port", + Have: 8080.0, Want: 9090.0, Changed: true, + }}, + DiffRuntime: []up.DiffRow{{ + Path: "containerGroups[default].replicaCount", + Have: 1.0, Want: 3.0, Changed: true, + }}, + + Unmanaged: []string{"containerGroups[default].containers[metrics]"}, + }, + } +} + +// TestCmd_JSONDiffSectionFollowsTheFlag threads the flag into the envelope +// itself: with it, stdout is still exactly one JSON document and the plan +// inside it gains the structured diff section; without it, the plan has no +// diff key at all, which is what keeps a consumer that never asked for a diff +// from having to learn about one. +func TestCmd_JSONDiffSectionFollowsTheFlag(t *testing.T) { + stubRun(t, diffedResult(), nil) + + stdout, _, err := runCmd(t, "--dry-run", "--output-format", "json", "--diff") + require.NoError(t, err) + + var envelope map[string]any + + require.NoError(t, json.Unmarshal([]byte(stdout), &envelope), + "stdout must be one JSON document and nothing else") + + body := envelope["up"].(map[string]any) + + plan, ok := body["plan"].(map[string]any) + require.True(t, ok) + + diff, ok := plan["diff"].(map[string]any) + require.True(t, ok, "--diff was requested, so the section is present") + + changes := diff["changes"].([]any) + require.Len(t, changes, 2) + + first := changes[0].(map[string]any) + + assert.Equal(t, "containerGroups[default].containers[primary].port", first["path"]) + assert.InDelta(t, 8080.0, first["have"], 0) + assert.InDelta(t, 9090.0, first["want"], 0) + assert.Equal(t, false, first["absent"]) + + unmanaged := diff["unmanaged"].([]any) + require.Len(t, unmanaged, 1) + assert.Equal(t, "containerGroups[default].containers[metrics]", unmanaged[0]) + + // The legacy arrays keep their place beside the new section. + artifact := plan["artifact"].([]any) + require.Len(t, artifact, 1) + assert.Equal(t, "containerGroups[default].containers[primary].port: 8080 -> 9090", artifact[0]) + + stubRun(t, diffedResult(), nil) + + stdout, _, err = runCmd(t, "--dry-run", "--output-format", "json") + require.NoError(t, err) + + envelope = map[string]any{} + + require.NoError(t, json.Unmarshal([]byte(stdout), &envelope), + "stdout must be one JSON document and nothing else") + + body = envelope["up"].(map[string]any) + plan = body["plan"].(map[string]any) + + assert.NotContains(t, plan, "diff", "without the flag the section is absent, not null") +} + func TestCmd_IsRegisteredUnderWorkload(t *testing.T) { cmd := Cmd() @@ -684,3 +817,305 @@ func TestCmd_FailedDraftRunOmitsTheLockLine(t *testing.T) { assert.Contains(t, next, "dr workload logs 68b0c1d2e3f4a5b6c7d8e9f0", "the lines that can name the workload still do") } + +// TestCmd_ConfirmIsRegistered: the flag exists, is opt-in, and its help says +// the one thing a reader could not guess -- that under non-interactive +// conditions it is suppressed rather than refused, because a flag that fails +// a scripted run helps nobody. +func TestCmd_ConfirmIsRegistered(t *testing.T) { + lookup := Cmd().Flags().Lookup("confirm") + + require.NotNil(t, lookup, "the flag has to exist for --confirm to parse at all") + assert.False(t, lookup.Hidden) + assert.Equal(t, "false", lookup.DefValue, "confirming is opt-in") + + for _, term := range []string{ + "(y/N)", "--yes", "--output-format", "DATAROBOT_CLI_NON_INTERACTIVE", + "terminal", "suppressed", + } { + assert.Contains(t, lookup.Usage, term, "the suppression matrix is documented in the flag itself") + } +} + +// TestCmd_ConfirmReachesTheDeploy threads the flag into the deploy's options, +// and only when it was given: a run without it must not start asking. +func TestCmd_ConfirmReachesTheDeploy(t *testing.T) { + onATerminal(t) + + seen := stubRun(t, deployed(), nil) + + _, _, err := runCmdWithInput(t, "y\n", "--confirm") + require.NoError(t, err) + require.NotNil(t, seen.ConfirmApply, "the gate reaches the deploy wired up") + + agreed, err := seen.ConfirmApply("Apply this deploy?") + require.NoError(t, err) + assert.True(t, agreed, "the answer is read from stdin") + + defaults := stubRun(t, deployed(), nil) + + _, _, err = runCmd(t) + require.NoError(t, err) + assert.Nil(t, defaults.ConfirmApply, "without the flag there is no gate") +} + +// TestCmd_ConfirmGate_AnswersOnlyYes: the question is a y/N with the default +// on the safe side. Every spelling of yes proceeds; bare Enter, any other +// word, and a stdin that ends before an answer all decline. +func TestCmd_ConfirmGate_AnswersOnlyYes(t *testing.T) { + cases := []struct { + typed string + want bool + }{ + {"y\n", true}, + {"Y\n", true}, + {"yes\n", true}, + {"YES\n", true}, + {"Yes\n", true}, + {"yEs\n", true}, + {" y \n", true}, + {"\n", false}, + {"", false}, + {"n\n", false}, + {"N\n", false}, + {"no\n", false}, + {"nope\n", false}, + {"deploy\n", false}, + } + + for _, c := range cases { + t.Run(c.typed, func(t *testing.T) { + cmd := Cmd() + + var errOut bytes.Buffer + + cmd.SetErr(&errOut) + cmd.SetIn(strings.NewReader(c.typed)) + + ask := applyConfirm(cmd, true, false) + require.NotNil(t, ask) + + agreed, err := ask("Apply this deploy?") + require.NoError(t, err) + + assert.Equal(t, c.want, agreed) + assert.Contains(t, errOut.String(), "? Apply this deploy? (y/N)", + "the question goes to stderr with the default shown") + }) + } +} + +// TestCmd_ConfirmGate_NotInstalledWhenSuppressed: the builder itself is nil +// whenever there is nobody to ask, which is the suppression-over-error +// convention the roll confirm follows. +func TestCmd_ConfirmGate_NotInstalledWhenSuppressed(t *testing.T) { + cmd := Cmd() + + assert.Nil(t, applyConfirm(cmd, false, false), "no flag, no gate") + assert.Nil(t, applyConfirm(cmd, true, true), "non-interactive swallows the flag") + assert.Nil(t, applyConfirm(cmd, false, true), "and neither installs the other") +} + +// --yes is already the answer, so --confirm alongside it behaves as if the +// flag was not given: no question, the same deploy. +func TestCmd_ConfirmSuppressed_Yes(t *testing.T) { + onATerminal(t) + + seen := stubRun(t, deployed(), nil) + + stdout, stderr, err := runCmd(t, "--yes", "--confirm") + require.NoError(t, err) + + assert.Nil(t, seen.ConfirmApply, "--yes is the answer, so there is no gate to install") + assert.NotContains(t, stderr, "? Apply") + assert.Equal(t, "https://app.datarobot.com/workloads/68b0/\n", stdout, + "the suppressed run is byte-identical to one without --confirm") +} + +// The non-interactive environment variable says nobody is reading, and a +// question nobody reads would hang a pipeline forever. +func TestCmd_ConfirmSuppressed_NonInteractiveEnv(t *testing.T) { + onATerminal(t) + t.Setenv("DATAROBOT_CLI_NON_INTERACTIVE", "1") + + seen := stubRun(t, deployed(), nil) + + _, stderr, err := runCmd(t, "--confirm") + require.NoError(t, err) + + assert.Nil(t, seen.ConfirmApply) + assert.NotContains(t, stderr, "? Apply") +} + +// -o json implies non-interactive for the y/N gate specifically. The typed +// production confirm is a different question with different rules and is +// covered by TestCmd_JSONOutputStillHandsOverTheQuestion. +func TestCmd_ConfirmSuppressed_JSON(t *testing.T) { + onATerminal(t) + + seen := stubRun(t, deployed(), nil) + + stdout, stderr, err := runCmd(t, "--output-format", "json", "--confirm") + require.NoError(t, err) + + assert.Nil(t, seen.ConfirmApply) + assert.True(t, json.Valid([]byte(stdout)), "stdout stays one JSON document") + assert.NotContains(t, stderr, "Apply this deploy", "the y/N gate is the prompt that is suppressed") +} + +// A piped stdin has nobody behind it, which is the CI case even with no other +// non-interactive signal set. +func TestCmd_ConfirmSuppressed_PipedStdin(t *testing.T) { + seen := stubRun(t, deployed(), nil) + + _, stderr, err := runCmd(t, "--confirm") + require.NoError(t, err) + + assert.True(t, seen.NonInteractive) + assert.Nil(t, seen.ConfirmApply, "a piped stdin has nobody to ask") + assert.NotContains(t, stderr, "? Apply") +} + +// TestCmd_ConfirmInstallKeysOnStdinNotStderr: the gate follows stdin, not +// stderr. A run with a terminal stdin still asks when stderr is redirected, +// and the question lands in whatever the stderr is. +func TestCmd_ConfirmInstallKeysOnStdinNotStderr(t *testing.T) { + onATerminal(t) + decliningRun(t) + + stdout, stderr, err := runCmdWithInput(t, "y\n", "--confirm") + require.NoError(t, err) + + assert.Contains(t, stderr, "? Apply this deploy? (y/N)", "the question is on stderr") + assert.Equal(t, "https://app.datarobot.com/workloads/68b0/\n", stdout, + "stdout stays the endpoint channel") +} + +// decliningRun wires the deploy the way the real one answers the gate, so the +// prompt, the parser and the command's decline mapping are exercised as one +// flow rather than as three unit-tested pieces. +func decliningRun(t *testing.T) { + t.Helper() + + prev := runFn + + runFn = func(opts up.Options) (up.Result, error) { + ok, err := opts.ConfirmApply("Apply this deploy?") + if err != nil { + return up.Result{}, err + } + + if !ok { + // WorkloadID is set on purpose: a declined run of an existing + // workload is exactly the case where falling through to the + // renderer would leak the endpoint onto stdout. + return up.Result{WorkloadID: "68b0c1d2e3f4a5b6c7d8e9f0"}, up.ErrDeclined + } + + return deployed(), nil + } + + t.Cleanup(func() { runFn = prev }) +} + +// TestCmd_ConfirmDecline_NonzeroExit: a decline is a refusal, and the command +// says so with a nonzero exit and one short line on stderr. +func TestCmd_ConfirmDecline_NonzeroExit(t *testing.T) { + onATerminal(t) + decliningRun(t) + + for _, input := range []string{"\n", "n\n", "nope\n", ""} { + stdout, stderr, err := runCmdWithInput(t, input, "--confirm") + + require.Error(t, err, "answering %q must exit nonzero", input) + require.ErrorIs(t, err, up.ErrDeclined) + assert.Empty(t, stdout, "a declined run prints nothing on stdout") + assert.Contains(t, stderr, "declined", "the decline is said on stderr") + assert.Contains(t, stderr, "nothing was deployed") + assert.NotContains(t, stderr, "https://app.datarobot.com/workloads/68b0/", + "the endpoint of a workload the run left alone is not printed") + } +} + +// TestCmd_ConfirmDecline_StdoutEmpty is the stdout half on its own, because +// it is the half a script breaks on: whatever the decline's exit code, stdout +// must be byte-for-byte empty. +func TestCmd_ConfirmDecline_StdoutEmpty(t *testing.T) { + onATerminal(t) + decliningRun(t) + + stdout, _, err := runCmdWithInput(t, "n\n", "--confirm") + require.Error(t, err) + assert.Empty(t, stdout) +} + +// TestCmd_ConfirmAccept_ProceedsToEndOfRun: accepting the y/N runs the normal +// success path, which in the shell means the endpoint lands on stdout as it +// always does. +func TestCmd_ConfirmAccept_ProceedsToEndOfRun(t *testing.T) { + onATerminal(t) + decliningRun(t) + + stdout, stderr, err := runCmdWithInput(t, "y\n", "--confirm") + require.NoError(t, err) + assert.Contains(t, stderr, "? Apply this deploy? (y/N)") + assert.Equal(t, "https://app.datarobot.com/workloads/68b0/\n", stdout) +} + +// TestCmd_ConfirmHelpDocumentsSuppression: the Long paragraph and the flag +// help between them tell a first-time reader what --diff prints, what +// --confirm asks, and when the asking is skipped. +func TestCmd_ConfirmHelpDocumentsSuppression(t *testing.T) { + long := Cmd().Long + + assert.Contains(t, long, "--diff") + assert.Contains(t, long, "--confirm") + assert.Contains(t, long, "--dry-run --diff", "the look-without-touching combination is named") + assert.Contains(t, long, "Only fields the manifest mentions are managed", + "the pre-existing narrative stays") + + lookup := Cmd().Flags().Lookup("confirm") + require.NotNil(t, lookup) + assert.Contains(t, lookup.Usage, "behaves as if", "the suppressed semantics are stated, not implied") +} + +// TestCmd_ConfirmAndDiffCompose: the two flags are independent, and cobra is +// deliberately not taught to refuse the pair -- suppression composes, mutual +// exclusivity does not. +func TestCmd_ConfirmAndDiffCompose(t *testing.T) { + onATerminal(t) + + seen := stubRun(t, deployed(), nil) + + _, _, err := runCmd(t, "--diff", "--confirm", "--dry-run") + require.NoError(t, err, "the flags compose; refusing the pair would break a scripted caller") + assert.True(t, seen.Diff) + assert.True(t, seen.DryRun) + assert.NotNil(t, seen.ConfirmApply) + + for _, name := range []string{"diff", "confirm"} { + lookup := Cmd().Flags().Lookup(name) + require.NotNil(t, lookup) + + assert.NotContains(t, lookup.Annotations, "cobra_annotation_mutually_exclusive", + "--diff and --confirm are never made mutually exclusive") + } +} + +// TestCmd_TelemetryRecordsTheConfirmFlag: adoption of the flag has to be +// readable under its own key, reflecting the flag as given rather than the +// suppressed effective behavior. +func TestCmd_TelemetryRecordsTheConfirmFlag(t *testing.T) { + withFlag := Cmd() + require.NoError(t, withFlag.ParseFlags([]string{"--confirm"})) + + event, ok := telemetry.EventFor(withFlag, nil) + require.True(t, ok, "EventFor must return ok=true for an annotated command") + + assert.Equal(t, true, event.EventProperties["confirm"]) + assert.Equal(t, false, event.EventProperties["diff"], "the pre-existing keys keep their own values") + + event, ok = telemetry.EventFor(Cmd(), nil) + require.True(t, ok) + assert.Equal(t, false, event.EventProperties["confirm"], "an unset flag reports itself as off, not as absent") +} diff --git a/go.mod b/go.mod index 6dfdfc288..87e549c70 100644 --- a/go.mod +++ b/go.mod @@ -22,6 +22,7 @@ require ( github.com/jeandeaual/go-locale v0.0.0-20250612000132-0ef82f21eade github.com/joho/godotenv v1.5.1 github.com/muesli/cancelreader v0.2.2 + github.com/muesli/termenv v0.16.0 github.com/sabhiram/go-gitignore v0.0.0-20210923224102-525f6e181f06 github.com/sergi/go-diff v1.4.0 github.com/spf13/cobra v1.10.2 @@ -69,7 +70,6 @@ require ( github.com/microcosm-cc/bluemonday v1.0.27 // indirect github.com/muesli/ansi v0.0.0-20230316100256-276c6243b2f6 // indirect github.com/muesli/reflow v0.3.0 // indirect - github.com/muesli/termenv v0.16.0 // indirect github.com/pelletier/go-toml/v2 v2.4.2 // indirect github.com/rivo/uniseg v0.4.7 // indirect github.com/sagikazarmark/locafero v0.12.0 // indirect diff --git a/internal/uidiff/render.go b/internal/uidiff/render.go new file mode 100644 index 000000000..992390e28 --- /dev/null +++ b/internal/uidiff/render.go @@ -0,0 +1,203 @@ +// Copyright 2026 DataRobot, Inc. and its affiliates. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package uidiff + +import ( + "fmt" + "io" + "strconv" + "strings" + + "github.com/charmbracelet/lipgloss" + "github.com/datarobot/cli/tui" +) + +// defaultContextWindow is the context window git taught everyone to read: +// enough surrounding rows to place a change, not so many that the diff stops +// being shorter than the thing it diffs. +const defaultContextWindow = 3 + +// The marker characters are the vocabulary the plan block already prints, so +// a diff and the default plan read as one visual language. Context and +// unmanaged rows add no marker: their text arrives from the caller complete. +const ( + addMarker = "+" + delMarker = "-" +) + +// The words that stand in for a value the redaction hook refused to print. +// "set" and "changed" are the plan's own vocabulary for a value arriving and +// a value moving; the bracketed token marks rows whose value is simply +// withheld, where neither verb is true. +const ( + setPlaceholder = "set" + changedPlaceholder = "changed" + hiddenPlaceholder = "(redacted)" +) + +// Render writes rows as a unified diff to w. +// +// Every change shows, and so does every unchanged row within the context +// window of one. The rest of each unchanged run collapses into a single +// "... N identical lines" line that counts only the rows it hides, so the +// rendering is always shorter than the row list it came from and never +// understates how much it left out. Redacted rows render as a placeholder +// built from their path, whatever their Kind: no value text reaches the +// writer for a path the hook refuses. +func Render(w io.Writer, rows []Row, opts Options) error { + window := opts.Context + if window <= 0 { + window = defaultContextWindow + } + + shown := shownRows(rows, window) + + var b strings.Builder + + for i := 0; i < len(rows); { + if shown[i] { + b.WriteString(renderRow(rows[i], opts.Redact)) + b.WriteString("\n") + + i++ + + continue + } + + hidden := 0 + + for i+hidden < len(rows) && !shown[i+hidden] { + hidden++ + } + + b.WriteString(collapseLine(hidden)) + b.WriteString("\n") + + i += hidden + } + + if _, err := io.WriteString(w, b.String()); err != nil { + return fmt.Errorf("cannot write diff: %w", err) + } + + return nil +} + +// shownRows marks which rows the window keeps: every change, plus every row +// within the window of one. Distance is measured in rows, not in equality, +// because the collapse answers "how much of this did I stop reading?" and +// that question does not care what the hidden rows say. +func shownRows(rows []Row, window int) []bool { + shown := make([]bool, len(rows)) + + for i, row := range rows { + if row.Kind != Add && row.Kind != Del { + continue + } + + lo := max(i-window, 0) + hi := min(i+window, len(rows)-1) + + for j := lo; j <= hi; j++ { + shown[j] = true + } + } + + return shown +} + +// collapseLine is the one line that stands for a hidden run. It counts the +// rows it hides rather than the ones it kept, so a reader can always tell +// how much output did not happen. +func collapseLine(hidden int) string { + return tui.HintStyle.Render("... " + strconv.Itoa(hidden) + " identical lines") +} + +// renderRow renders one shown row: the marker and text its kind calls for, +// with the redaction hook standing in for the text of a row whose value must +// not print. +func renderRow(row Row, redact func(string) bool) string { + marker, text := rowContent(row, redact) + + if marker == "" { + return rowStyle(row.Kind).Render(text) + } + + return rowStyle(row.Kind).Render(marker + " " + text) +} + +// rowContent decides what a row prints. The caller's Text is the answer for +// everything but a redacted path, where the value inside the caller's text +// is replaced by the placeholder its kind calls for: the path stays, the +// value never does. The hook is consulted once per row, here, so a caller +// with an expensive predicate pays for it once. +func rowContent(row Row, redact func(string) bool) (marker, text string) { + text = row.Text + + if redact != nil && redact(row.Path) { + text = redactedText(row) + } + + switch row.Kind { + case Add: + return addMarker, text + case Del: + return delMarker, text + case Context, Unmanaged: + return "", text + } + + // An out-of-range Kind has no marker of its own; it renders as context + // rather than inventing one. + return "", text +} + +// redactedText is what a row prints when its value must not: the path, plus +// the word its kind calls for. "set" and "changed" are the plan's own +// vocabulary for a value arriving and a value moving; the bracketed token +// marks rows whose value is simply withheld, where neither verb is true. +func redactedText(row Row) string { + switch row.Kind { + case Add: + return row.Path + ": " + setPlaceholder + case Del: + return row.Path + ": " + changedPlaceholder + case Context, Unmanaged: + return row.Path + ": " + hiddenPlaceholder + } + + // An out-of-range Kind falls through the switch the way Context does, + // and for the same reason: "set" and "changed" are claims about a + // transition the renderer recognizes, and it makes neither for a Kind it + // does not. + return row.Path + ": " + hiddenPlaceholder +} + +// rowStyle is the palette entry for a row's kind. An addition reads as new, +// a replacement and an unmanaged field as something being moved, and +// everything merely contextual as commentary on it. +func rowStyle(kind Kind) lipgloss.Style { + switch kind { + case Add: + return tui.SuccessStyle + case Del, Unmanaged: + return tui.WarnStyle + case Context: + return tui.HintStyle + } + + // An out-of-range Kind reads as context, quieter than guessing wrong. + return tui.HintStyle +} diff --git a/internal/uidiff/render_test.go b/internal/uidiff/render_test.go new file mode 100644 index 000000000..76a10ce29 --- /dev/null +++ b/internal/uidiff/render_test.go @@ -0,0 +1,418 @@ +// Copyright 2026 DataRobot, Inc. and its affiliates. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package uidiff + +import ( + "errors" + "regexp" + "strings" + "testing" + + "github.com/charmbracelet/lipgloss" + "github.com/muesli/termenv" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/datarobot/cli/tui" +) + +// renderRows runs Render against a buffer, which is how every test below +// reads a rendering. +func renderRows(t *testing.T, rows []Row, opts Options) string { + t.Helper() + + var b strings.Builder + + require.NoError(t, Render(&b, rows, opts)) + + return b.String() +} + +// stripANSI removes lipgloss colour escapes so assertions can match plain +// text. Styling is environment dependent (it varies with terminal colour +// support), so tests assert on content, never on the escape sequences. That +// is also the parseability guarantee: with styles stripped the markers are +// still there, because they are ordinary characters the styling wrapped. +var ansiPattern = regexp.MustCompile(`\x1b\[[0-9;]*m`) + +func stripANSI(text string) string { + return ansiPattern.ReplaceAllString(text, "") +} + +// ctxRow, addRow and delRow build rows whose visible text is the argument +// itself, so an expected rendering reads as the rows it came from. +func ctxRow(text string) Row { + return Row{Kind: Context, Path: text, Text: text} +} + +func addRow(text string) Row { + return Row{Kind: Add, Path: text, Text: text} +} + +func delRow(text string) Row { + return Row{Kind: Del, Path: text, Text: text} +} + +// extraRow is an unmanaged row the way the workload caller emits them: the +// leading marker and the wording are the caller's, carried in Text, and the +// renderer adds nothing of its own. +func extraRow(path string) Row { + return Row{Kind: Unmanaged, Path: path, Text: "~ " + path + ": not managed by this file"} +} + +// VAL-DIFF-001: the window is exactly three unchanged rows either side of a +// change, and everything beyond it on both sides collapses. +func TestRender_ContextWindowIsThreeEachSide(t *testing.T) { + rows := []Row{ + ctxRow("before-1"), ctxRow("before-2"), ctxRow("before-3"), + ctxRow("before-4"), ctxRow("before-5"), ctxRow("before-6"), + addRow("the change"), + ctxRow("after-1"), ctxRow("after-2"), ctxRow("after-3"), + ctxRow("after-4"), ctxRow("after-5"), ctxRow("after-6"), + } + + out := renderRows(t, rows, Options{}) + + assert.Equal(t, + "... 3 identical lines\n"+ + "before-4\nbefore-5\nbefore-6\n"+ + "+ the change\n"+ + "after-1\nafter-2\nafter-3\n"+ + "... 3 identical lines\n", + stripANSI(out)) +} + +// A side that cannot fill the window shows what is there rather than padding. +func TestRender_ChangeWithFewerContextRowsShowsWhatIsThere(t *testing.T) { + rows := []Row{ + ctxRow("only-1"), ctxRow("only-2"), + delRow("the change"), + ctxRow("after"), + } + + out := renderRows(t, rows, Options{}) + + assert.Equal(t, + "only-1\nonly-2\n- the change\nafter\n", + stripANSI(out)) +} + +// VAL-DIFF-002: a run at exactly the window shows in full, no collapse line. +func TestRender_RunAtTheWindowIsShownInFull(t *testing.T) { + rows := []Row{ + addRow("first change"), + ctxRow("between-1"), ctxRow("between-2"), ctxRow("between-3"), + delRow("second change"), + } + + out := stripANSI(renderRows(t, rows, Options{})) + + assert.Equal(t, + "+ first change\nbetween-1\nbetween-2\nbetween-3\n- second change\n", + out) + assert.NotContains(t, out, "...", "a run at the window never collapses") +} + +// One row past the window: the three change-adjacent rows survive and the +// single farthest one collapses, counted exactly. +func TestRender_RunOnePastTheWindowCollapses(t *testing.T) { + rows := []Row{ + ctxRow("far"), ctxRow("near-1"), ctxRow("near-2"), ctxRow("near-3"), + addRow("the change"), + } + + out := renderRows(t, rows, Options{}) + + assert.Equal(t, + "... 1 identical lines\nnear-1\nnear-2\nnear-3\n+ the change\n", + stripANSI(out)) +} + +// Two changes seven rows apart: each keeps its three, and the one middle row +// neither side reaches is the whole hidden count. +func TestRender_SevenBetweenTwoChangesHidesTheMiddleOne(t *testing.T) { + rows := []Row{ + addRow("first"), + ctxRow("c1"), ctxRow("c2"), ctxRow("c3"), + ctxRow("c4"), ctxRow("c5"), ctxRow("c6"), ctxRow("c7"), + delRow("second"), + } + + out := renderRows(t, rows, Options{}) + + assert.Equal(t, + "+ first\nc1\nc2\nc3\n... 1 identical lines\nc5\nc6\nc7\n- second\n", + stripANSI(out)) +} + +// Two long runs in one body collapse separately, each line counting only its +// own run. +func TestRender_TwoLongRunsCollapseIndependently(t *testing.T) { + rows := []Row{ + ctxRow("lead-1"), ctxRow("lead-2"), ctxRow("lead-3"), + ctxRow("lead-4"), ctxRow("lead-5"), + addRow("the change"), + ctxRow("tail-1"), ctxRow("tail-2"), ctxRow("tail-3"), + ctxRow("tail-4"), ctxRow("tail-5"), + } + + out := renderRows(t, rows, Options{}) + + assert.Equal(t, + "... 2 identical lines\nlead-3\nlead-4\nlead-5\n"+ + "+ the change\n"+ + "tail-1\ntail-2\ntail-3\n... 2 identical lines\n", + stripANSI(out)) +} + +// VAL-DIFF-003: runs touching the ends of the list collapse away from the +// change, on the far side of the rows that survive. +func TestRender_ListEdgeRunsCollapseAwayFromTheChange(t *testing.T) { + leading := renderRows(t, []Row{ + ctxRow("c1"), ctxRow("c2"), ctxRow("c3"), ctxRow("c4"), ctxRow("c5"), + addRow("the change"), + }, Options{}) + + assert.Equal(t, + "... 2 identical lines\nc3\nc4\nc5\n+ the change\n", + stripANSI(leading), + "the collapse line sits on the far side of a leading run, never the change side") + + trailing := renderRows(t, []Row{ + delRow("the change"), + ctxRow("c1"), ctxRow("c2"), ctxRow("c3"), ctxRow("c4"), ctxRow("c5"), + }, Options{}) + + assert.Equal(t, + "- the change\nc1\nc2\nc3\n... 2 identical lines\n", + stripANSI(trailing)) +} + +// A body with no change at all is one whole hidden run: the collapse line is +// the only output, and it counts everything. +func TestRender_AllContextCollapsesToOneLine(t *testing.T) { + out := renderRows(t, []Row{ + ctxRow("c1"), ctxRow("c2"), ctxRow("c3"), ctxRow("c4"), + }, Options{}) + + assert.Equal(t, "... 4 identical lines\n", stripANSI(out)) +} + +// VAL-DIFF-005: unmanaged rows run as context, carry the caller's marker +// from Text, and never render as a removal. +func TestRender_UnmanagedRowsRunWithContextAndNeverRenderAsRemovals(t *testing.T) { + rows := []Row{ + ctxRow("c1"), ctxRow("c2"), + extraRow("environmentVars[SIDECAR]"), + extraRow("environmentVars[AUTOSCALE]"), + addRow("runtime.replicaCount: 3"), + } + + out := renderRows(t, rows, Options{}) + + assert.Equal(t, + "... 1 identical lines\nc2\n"+ + "~ environmentVars[SIDECAR]: not managed by this file\n"+ + "~ environmentVars[AUTOSCALE]: not managed by this file\n"+ + "+ runtime.replicaCount: 3\n", + stripANSI(out)) +} + +// The markers are the vocabulary the plan block already prints, so a diff +// and the default plan read as one visual language. +func TestRender_MarkersFollowThePlanVocabulary(t *testing.T) { + rows := []Row{ + ctxRow("unchanged"), + delRow("runtime.replicaCount: 1"), + addRow("runtime.replicaCount: 3"), + extraRow("environmentVars[SIDECAR]"), + } + + out := renderRows(t, rows, Options{}) + + assert.Equal(t, + "unchanged\n"+ + "- runtime.replicaCount: 1\n"+ + "+ runtime.replicaCount: 3\n"+ + "~ environmentVars[SIDECAR]: not managed by this file\n", + stripANSI(out)) +} + +// VAL-DIFF-004: the redaction hook suppresses the value of every Kind it +// fires for, whatever the row was carrying, and touches nothing else. This +// table is the canonical Kind x redaction-state matrix: every Kind appears +// in each state the hook admits -- fired, passed over, and absent (nil +// hook) -- so no combination ships untested and the placeholder vocabulary +// has exactly one home. +func TestRender_RedactionAppliesToEveryKind(t *testing.T) { + const ( + path = "runtime.environmentVars[TOKEN]" + secret = "plaintext-secret" + ) + + // The marker and placeholder each Kind earns: verbs for a value arriving + // and a value moving, the bracketed token where neither verb is true, + // and a marker only for the kinds that announce a change. + kinds := []struct { + name string + kind Kind + marker string + placeholder string + }{ + {name: "context", kind: Context, placeholder: hiddenPlaceholder}, + {name: "add", kind: Add, marker: addMarker, placeholder: setPlaceholder}, + {name: "del", kind: Del, marker: delMarker, placeholder: changedPlaceholder}, + {name: "unmanaged", kind: Unmanaged, placeholder: hiddenPlaceholder}, + } + + states := []struct { + name string + hook func(string) bool + redacted bool + }{ + // The firing hook is path-scoped, the way the workload caller's is: + // it refuses the row's path, not the whole rendering, so the anchor + // row keeps its value even here. + {name: "hook-fires", hook: func(p string) bool { return p == path }, redacted: true}, + {name: "hook-passes", hook: func(string) bool { return false }, redacted: false}, + {name: "no-hook", hook: nil, redacted: false}, + } + + for _, k := range kinds { + for _, s := range states { + t.Run(k.name+"/"+s.name, func(t *testing.T) { + // Context and Unmanaged rows only show inside the window of + // a change, so every case anchors its row to one; the + // anchor's line is part of each expectation. + rows := []Row{ + {Kind: k.kind, Path: path, Text: path + ": " + secret}, + {Kind: Add, Path: "anchor", Text: "anchor"}, + } + + var opts Options + + if s.hook != nil { + opts.Redact = s.hook + } + + out := stripANSI(renderRows(t, rows, opts)) + + marker := k.marker + if marker != "" { + marker += " " + } + + want := marker + path + ": " + secret + if s.redacted { + want = marker + path + ": " + k.placeholder + + assert.NotContains(t, out, secret, "a fired hook never lets the value through") + } + + assert.Equal(t, want+"\n+ anchor\n", out) + }) + } + } +} + +// The style mapping is invisible to the other tests because they strip ANSI: +// they prove what a row says, never what it wears. This one forces a color +// profile so the escapes survive, then checks each Kind against the palette +// entry it is documented to carry -- additions read as success, removals and +// unmanaged fields as warnings, and context as a hint. The expected strings +// come from the same styles the renderer uses, so the assertion holds +// whatever colors the profile downsamples to. +func TestRender_PerKindStylesWithForcedColorProfile(t *testing.T) { + original := lipgloss.ColorProfile() + + lipgloss.SetColorProfile(termenv.ANSI) + + t.Cleanup(func() { + lipgloss.SetColorProfile(original) + }) + + rows := []Row{ + {Kind: Context, Path: "ctx", Text: "ctx"}, + {Kind: Add, Path: "added", Text: "added"}, + {Kind: Del, Path: "removed", Text: "removed"}, + {Kind: Unmanaged, Path: "extra", Text: "~ extra: not managed"}, + } + + out := renderRows(t, rows, Options{}) + + // A profile that failed to engage would leave both sides of the checks + // below plain text and the whole test vacuous, so first prove that the + // rendering actually carries escapes. + assert.NotEqual(t, stripANSI(out), out, "forcing a color profile must leave ANSI escapes in the rendering") + + assert.Contains(t, out, tui.SuccessStyle.Render("+ added"), "Add wears the success style") + assert.Contains(t, out, tui.WarnStyle.Render("- removed"), "Del wears the warn style") + assert.Contains(t, out, tui.WarnStyle.Render("~ extra: not managed"), "Unmanaged wears the warn style, like Del") + assert.Contains(t, out, tui.HintStyle.Render("ctx"), "Context wears the hint style") +} + +// The hook is a per-row decision: once per row, never per line of output. +func TestRender_RedactHookIsConsultedOncePerRow(t *testing.T) { + calls := 0 + + rows := []Row{ctxRow("a"), addRow("b"), delRow("c"), extraRow("d"), ctxRow("e")} + + var b strings.Builder + + require.NoError(t, Render(&b, rows, Options{Redact: func(string) bool { + calls++ + + return false + }})) + + assert.Equal(t, len(rows), calls, "the hook runs once per row") +} + +// VAL-DIFF-001: zero means the default window, and the Context field is how +// a caller asks for a different one. +func TestRender_ZeroContextMeansTheDefaultWindow(t *testing.T) { + rows := []Row{ + ctxRow("c1"), ctxRow("c2"), ctxRow("c3"), ctxRow("c4"), ctxRow("c5"), + addRow("the change"), + } + + assert.Equal(t, + renderRows(t, rows, Options{Context: 3}), + renderRows(t, rows, Options{}), + "zero and three are the same window") + + out := renderRows(t, rows, Options{Context: 1}) + + assert.Equal(t, "... 4 identical lines\nc5\n+ the change\n", stripANSI(out)) +} + +// An empty body is an empty rendering, not a stray newline. +func TestRender_NoRowsWritesNothing(t *testing.T) { + assert.Empty(t, renderRows(t, nil, Options{})) +} + +var errWriteFailed = errors.New("write failed") + +type failingWriter struct{} + +func (failingWriter) Write([]byte) (int, error) { return 0, errWriteFailed } + +// A writer that fails leaves the caller holding the error; a diff that +// reported success while losing lines would be worse than one that failed. +func TestRender_PropagatesWriteErrors(t *testing.T) { + err := Render(failingWriter{}, []Row{ctxRow("c")}, Options{}) + + require.ErrorIs(t, err, errWriteFailed) +} diff --git a/internal/uidiff/uidiff.go b/internal/uidiff/uidiff.go new file mode 100644 index 000000000..d72cafd17 --- /dev/null +++ b/internal/uidiff/uidiff.go @@ -0,0 +1,78 @@ +// Copyright 2026 DataRobot, Inc. and its affiliates. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package uidiff turns ordered rows into unified-diff-flavored output: the +// three-line context window git made conventional, collapsing of unchanged +// runs longer than that window, line styling from the CLI's shared palette, +// and a caller-supplied hook that keeps a value its owner refuses to print +// out of the output entirely. +// +// The package knows nothing about what the rows describe. Workloads, +// manifests and their name-keyed lists are the caller's business: the caller +// hands over rows whose text it has already formatted, and a predicate +// saying which paths carry values that must never reach the screen. What +// makes a row an addition, a replacement, context, or unmanaged field is +// likewise the caller's judgment; this package only decides what such a row +// looks like. +package uidiff + +// Kind classifies one row of a diff body. +type Kind int + +const ( + // Context is a row the caller counts as unchanged. Context rows show + // within the window of a change and collapse into a summary beyond it. + Context Kind = iota + + // Add is a value the caller is introducing, rendered with a "+". + Add + + // Del is a value the caller is replacing, rendered with a "-". + Del + + // Unmanaged is a row the caller wants visible but does not manage, such + // as a field the live object carries that the file never names. It + // renders as context, never as a removal, and the caller's Text carries + // its marker. + Unmanaged +) + +// Row is one line of a diff body. +type Row struct { + // Kind selects the marker and style the renderer applies. + Kind Kind + + // Path identifies what the row describes. It exists for the redaction + // hook and nothing else; the visible text comes from Text. + Path string + + // Text is the line's content, already formatted by the caller. For + // Unmanaged rows the leading marker is part of the Text, since only the + // caller knows the vocabulary the row needs. + Text string +} + +// Options tunes one Render call. The zero value is ready to use: the context +// window defaults to three and nothing is redacted. +type Options struct { + // Context is how many unchanged rows each side of a change stay visible. + // Zero means the default, three. There is no CLI flag for it, by design: + // a diff window is a rendering convention, not a knob. + Context int + + // Redact reports whether a path's value must never reach the output. A + // path it reports true for renders as a placeholder in place of its + // Text, whatever the row's Kind. Nil means nothing is redacted. + Redact func(path string) bool +} diff --git a/internal/workload/up/confirm_test.go b/internal/workload/up/confirm_test.go new file mode 100644 index 000000000..df0d7291d --- /dev/null +++ b/internal/workload/up/confirm_test.go @@ -0,0 +1,651 @@ +// Copyright 2026 DataRobot, Inc. and its affiliates. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package up + +import ( + "bytes" + "encoding/json" + "errors" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/datarobot/cli/internal/workload" + "github.com/datarobot/cli/internal/workload/sync" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// runConfirmIn is runIn with the stderr buffer supplied by the test, so the +// gate's recorder can measure what had been printed by the time the question +// was asked. Placement is most of what the gate has to get right, and offsets +// are how a buffer answers questions about order. +func runConfirmIn(t *testing.T, content string, opts Options, stderr *bytes.Buffer) (Result, error) { + t.Helper() + + dir := t.TempDir() + writeManifest(t, dir, content) + + // The validator refuses a provided-source build with no Dockerfile beside + // the manifest, which is correct and means the build-track fixtures need + // a real one. Writing it unconditionally is harmless for the others. + require.NoError(t, os.WriteFile(filepath.Join(dir, "Dockerfile"), + []byte("FROM scratch\nEXPOSE 8080\n"), 0o600)) + + opts.Dir = dir + opts.Stderr = stderr + + return Run(opts) +} + +// liveDocs is the bound pair the empty-plan and retune fixtures deploy +// against, and the decline tests wire counters on top of it. +func liveDocs(t *testing.T) fakes { + return fakes{ + workloadD: func(string) (workload.Document, error) { return doc(t, liveWorkloadJSON), nil }, + artifactD: func(string) (workload.Document, error) { return doc(t, liveArtifactJSON), nil }, + } +} + +// TestRun_ConfirmPromptsAfterPlan is the go-test half of the prompt contract: +// the gate asks once, after the plan has been rendered, and the question is +// the one the y/N answer is graded against. The decline also pins the run's +// own half of the exit contract: ErrDeclined, from a run that has printed the +// plan and mutated nothing. +func TestRun_ConfirmPromptsAfterPlan(t *testing.T) { + install(t, liveDocs(t)) + + var ( + asked int + question string + printed int + ) + + var stderr bytes.Buffer + + result, err := runConfirmIn(t, resizedManifest(), Options{ + ConfirmApply: func(q string) (bool, error) { + asked++ + question = q + printed = stderr.Len() + + return false, nil + }, + }, &stderr) + + require.ErrorIs(t, err, ErrDeclined) + assert.Equal(t, 1, asked, "the gate is asked once, not once per branch") + assert.Equal(t, "Apply this deploy?", question) + assert.Contains(t, stderr.String()[:printed], "-> ", + "the default plan precedes the question: consent follows the review") + assert.Equal(t, ActionUnchanged, result.Action, "a decline did nothing, so nothing is reported") +} + +// TestRun_ConfirmGate_AfterDryRunReturn: a preview is not a mutation to +// consent to, so the dry-run return comes first and the gate is never armed. +func TestRun_ConfirmGate_AfterDryRunReturn(t *testing.T) { + f := liveDocs(t) + f.settings = func(string, json.RawMessage) (*workload.Replacement, error) { + t.Fatal("a dry run must not touch the workload") + + return nil, nil + } + + install(t, f) + + var asked bool + + result, stderr, err := runIn(t, resizedManifest(), Options{ + NonInteractive: true, + DryRun: true, + ConfirmApply: func(string) (bool, error) { asked = true; return false, nil }, + }) + + require.NoError(t, err) + assert.False(t, asked, "dry-run never prompts") + assert.Equal(t, ActionUpdated, result.Action, "the dry run's action is still the plan's own") + assert.Contains(t, stderr, "-> ", "the plan is printed as usual") +} + +// TestRun_ConfirmGate_AfterNoteUnusedForce: the --force-build note is part of +// what the user reviews, so it is said before the question, not after it. +func TestRun_ConfirmGate_AfterNoteUnusedForce(t *testing.T) { + install(t, liveDocs(t)) + + var printed int + + var stderr bytes.Buffer + + _, err := runConfirmIn(t, resizedManifest(), Options{ + ForceBuild: true, + ConfirmApply: func(string) (bool, error) { printed = stderr.Len(); return false, nil }, + }, &stderr) + + require.ErrorIs(t, err, ErrDeclined) + + note := strings.Index(stderr.String(), "--force-build had no effect") + require.NotEqual(t, -1, note, "the unused-force note is part of the output") + assert.Less(t, note, printed, "the note precedes the question") +} + +// TestRun_ConfirmAccept_Proceeds: yes means the run continues through the +// normal apply path exactly as if the gate had never been installed. The seam +// sequence is captured twice -- once without the gate, once accepting it -- +// and the two must agree, because --confirm is a question, not a new plan. +func TestRun_ConfirmAccept_Proceeds(t *testing.T) { + seams := func(seen *[]string) fakes { + f := liveDocs(t) + f.settings = func(string, json.RawMessage) (*workload.Replacement, error) { + *seen = append(*seen, "resize") + + return nil, nil + } + + return f + } + + var baseline []string + + install(t, seams(&baseline)) + + _, _, err := runIn(t, resizedManifest(), Options{NonInteractive: true, Detach: true}) + require.NoError(t, err) + require.NotEmpty(t, baseline, "the fixture has to reach the resize for the comparison to mean anything") + + var accepted []string + + install(t, seams(&accepted)) + + result, _, err := runIn(t, resizedManifest(), Options{ + Detach: true, + ConfirmApply: func(string) (bool, error) { return true, nil }, + }) + + require.NoError(t, err) + assert.Equal(t, baseline, accepted, "accepting the gate proceeds exactly as a run without it") + assert.Equal(t, ActionUpdated, result.Action) +} + +// countingFakes wires a named marker into every seam the contract counts as a +// mutation, so a test asserting len(calls) == 0 is asserting the run touched +// none of them. +func countingFakes(calls *[]string) func(f fakes) fakes { + mark := func(name string) { *calls = append(*calls, name) } + + return func(f fakes) fakes { + f.cred = func(string) (*workload.Credential, error) { mark("cred"); return nil, nil } + f.findCredential = func(string, int) (*workload.Credential, error) { mark("findCredential"); return nil, nil } + f.guard = func(string) error { mark("guard"); return nil } + f.lock = func(id string) (*workload.Artifact, error) { + mark("lock:" + id) + + return &workload.Artifact{ID: id, Status: workload.ArtifactStatusLocked}, nil + } + f.create = func(any) (*workload.Workload, error) { mark("create"); return running("wl-new"), nil } + f.start = func(string) (*workload.WorkloadOperationResponse, error) { mark("start"); return nil, nil } + f.writeID = func(string, string) error { mark("writeID"); return nil } + f.build = func(string) (*workload.BuildTriggerResponse, error) { mark("build"); return nil, nil } + f.codeRef = func(string, string, string) error { mark("codeRef"); return nil } + f.sync = func(string) (*sync.Result, error) { mark("sync"); return nil, nil } + f.settings = func(string, json.RawMessage) (*workload.Replacement, error) { mark("resize"); return nil, nil } + f.replace = func(string, string, json.RawMessage) (*workload.Replacement, error) { mark("replace"); return nil, nil } + + return f + } +} + +// TestRun_ConfirmDecline_MutatesNothing is the promise the gate exists to +// keep: every decline answer stops the run with ErrDeclined and not one +// mutating seam has fired. The plan is printed first -- the user is declining +// something they read -- and the action stays "nothing". An answer that +// cannot be read at all is a different failure: the run stops too, but it is +// not a decline, so the sentinel stays out of it. +func TestRun_ConfirmDecline_MutatesNothing(t *testing.T) { + cases := []struct { + name string + answer bool + readErr error + declined bool + }{ + {name: "no", answer: false, declined: true}, + {name: "unreadable", readErr: errors.New("stdin closed")}, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + var calls []string + + f := countingFakes(&calls)(liveDocs(t)) + + install(t, f) + + result, stderr, err := runIn(t, resizedManifest(), Options{ + ConfirmApply: func(string) (bool, error) { return c.answer, c.readErr }, + }) + + require.Error(t, err) + + if c.declined { + require.ErrorIs(t, err, ErrDeclined) + } else { + require.NotErrorIs(t, err, ErrDeclined, + "a failed read is not a decline, and must not read as one") + assert.Contains(t, err.Error(), "stdin closed") + } + + assert.Empty(t, calls, "neither answer reaches any mutating seam") + assert.Contains(t, stderr, "-> ", "the plan precedes the question") + assert.Equal(t, ActionUnchanged, result.Action) + }) + } +} + +// TestRun_ConfirmDecline_NoApplyMutation names the apply half of the +// non-mutation promise explicitly: on a non-empty plan, declining stops +// before the credential checks, the rollout guard and the apply itself. +func TestRun_ConfirmDecline_NoApplyMutation(t *testing.T) { + var calls []string + + f := countingFakes(&calls)(liveDocs(t)) + f.replace = func(string, string, json.RawMessage) (*workload.Replacement, error) { + calls = append(calls, "replace") + + return nil, nil + } + + install(t, f) + + _, _, err := runIn(t, resizedManifest(), Options{ + ConfirmApply: func(string) (bool, error) { return false, nil }, + }) + + require.ErrorIs(t, err, ErrDeclined) + + for _, seam := range []string{"cred", "findCredential", "guard", "resize", "replace", "create", "build"} { + assert.NotContains(t, calls, seam, "%s must not fire past a declined gate", seam) + } +} + +// TestRun_ConfirmDecline_NoBuild: a genuine build is a mutation like any +// other, and a decline leaves the image untriggered. +func TestRun_ConfirmDecline_NoBuild(t *testing.T) { + var calls []string + + install(t, countingFakes(&calls)(fakes{})) + + _, _, err := runIn(t, unboundDockerfileManifest, Options{ + ConfirmApply: func(string) (bool, error) { return false, nil }, + }) + + require.ErrorIs(t, err, ErrDeclined) + assert.NotContains(t, calls, "build") + assert.NotContains(t, calls, "create") +} + +// TestRun_ConfirmDecline_NoLockOnlyMutation: an empty plan with --lock would +// still make the serving artifact permanent, and a decline has to stop before +// that one-way door too. +func TestRun_ConfirmDecline_NoLockOnlyMutation(t *testing.T) { + var calls []string + + f := countingFakes(&calls)(fakes{ + workloadD: func(string) (workload.Document, error) { return doc(t, liveWorkloadJSON), nil }, + artifactD: func(string) (workload.Document, error) { return draftArtifact(t), nil }, + }) + + install(t, f) + + bound := "workloadId: 68b0c1d2e3f4a5b6c7d8e9f0\n" + boundLiveManifest + + _, _, err := runIn(t, bound, Options{ + Lock: true, + ConfirmApply: func(string) (bool, error) { return false, nil }, + }) + + require.ErrorIs(t, err, ErrDeclined) + assert.NotContains(t, calls, "lock", "locking cannot be undone, so it waits for a yes like everything else") + assert.NotContains(t, calls, "guard") +} + +// TestRun_Confirm_LockOnlyPrompts: --lock is about the end state, so an empty +// plan with the flag pending still locks. Locking is a mutation, so the gate +// fires on it exactly as on a deploy: asked once, respected on both sides. +func TestRun_Confirm_LockOnlyPrompts(t *testing.T) { + bound := "workloadId: 68b0c1d2e3f4a5b6c7d8e9f0\n" + boundLiveManifest + + lockSeams := func(locked *[]string) fakes { + return fakes{ + workloadD: func(string) (workload.Document, error) { return doc(t, liveWorkloadJSON), nil }, + artifactD: func(string) (workload.Document, error) { return draftArtifact(t), nil }, + lock: func(id string) (*workload.Artifact, error) { + *locked = append(*locked, id) + + return &workload.Artifact{ID: id, Status: workload.ArtifactStatusLocked}, nil + }, + } + } + + var declined []string + + asked := 0 + + install(t, lockSeams(&declined)) + + _, _, err := runIn(t, bound, Options{ + Lock: true, + ConfirmApply: func(string) (bool, error) { + asked++ + + return false, nil + }, + }) + + require.ErrorIs(t, err, ErrDeclined) + assert.Equal(t, 1, asked, "an empty plan with a pending lock still prompts") + assert.Empty(t, declined, "the decline leaves the artifact as it is") + + var accepted []string + + asked = 0 + + install(t, lockSeams(&accepted)) + + result, _, err := runIn(t, bound, Options{ + Lock: true, + ConfirmApply: func(string) (bool, error) { + asked++ + + return true, nil + }, + }) + + require.NoError(t, err) + assert.Equal(t, 1, asked) + assert.Len(t, accepted, 1, "accepting proceeds into the lock") + assert.True(t, result.Locked) +} + +// TestRun_ConfirmDetach: --detach changes when the run returns, not whether +// it deploys, so the gate fires exactly as without it -- after the plan, +// before the apply. A decline still mutates nothing, and an accept goes out +// detached: the resize is requested and the run returns without waiting for +// it to serve. +func TestRun_ConfirmDetach(t *testing.T) { + var ( + calls []string + resized bool + ) + + counting := func(f fakes) fakes { + f.settings = func(string, json.RawMessage) (*workload.Replacement, error) { + calls = append(calls, "resize") + resized = true + + return nil, nil + } + f.waitSteady = func(string, time.Duration, time.Duration, + func(*workload.Workload), + ) (*workload.Workload, error) { + t.Fatal("--detach must not wait for the workload to settle") + + return nil, nil + } + + return f + } + + install(t, counting(liveDocs(t))) + + asked := 0 + + _, _, err := runIn(t, resizedManifest(), Options{ + Detach: true, + ConfirmApply: func(string) (bool, error) { asked++; return false, nil }, + }) + + require.ErrorIs(t, err, ErrDeclined) + assert.Equal(t, 1, asked, "the gate is asked on the detached path too") + assert.Empty(t, calls, "a decline on a detached run mutates nothing") + + install(t, counting(liveDocs(t))) + + result, _, err := runIn(t, resizedManifest(), Options{ + Detach: true, + ConfirmApply: func(string) (bool, error) { return true, nil }, + }) + + require.NoError(t, err) + assert.True(t, resized, "accepting proceeds into the detached apply") + assert.Equal(t, ActionUpdated, result.Action) +} + +// TestRun_Confirm_NoOpDoesNotPrompt: the gate fires whenever the run would +// otherwise mutate, and a wholly empty plan would not. Its output must not +// drift either way: --confirm on a no-op run is indistinguishable from a run +// without the flag. +func TestRun_Confirm_NoOpDoesNotPrompt(t *testing.T) { + f := liveDocs(t) + f.create = func(any) (*workload.Workload, error) { + t.Fatal("nothing changed, so nothing should be created") + + return nil, nil + } + + install(t, f) + + bound := "workloadId: 68b0c1d2e3f4a5b6c7d8e9f0\n" + boundLiveManifest + + _, baseline, err := runIn(t, bound, Options{NonInteractive: true}) + require.NoError(t, err) + + install(t, f) + + var asked bool + + _, stderr, err := runIn(t, bound, Options{ + NonInteractive: true, + ConfirmApply: func(string) (bool, error) { asked = true; return false, nil }, + }) + + require.NoError(t, err) + assert.False(t, asked, "nothing pending means nothing to consent to") + assert.Equal(t, baseline, stderr, "the empty-plan output does not drift when --confirm is given") +} + +// TestRun_Confirm_UnmanagedOnlyDoesNotPrompt: unmanaged fields are never +// mutated -- they survive every deploy untouched -- so a plan whose only +// content is unmanaged fields mutates nothing and must not arm the gate. The +// fixture is the empty-plan one, whose live object carries a sidecar, a +// resource bundle and an autoscaling policy the file never names. +func TestRun_Confirm_UnmanagedOnlyDoesNotPrompt(t *testing.T) { + install(t, liveDocs(t)) + + bound := "workloadId: 68b0c1d2e3f4a5b6c7d8e9f0\n" + boundLiveManifest + + result, baseline, err := runIn(t, bound, Options{NonInteractive: true}) + require.NoError(t, err) + require.NotEmpty(t, result.Plan.Unmanaged, + "the fixture has to carry unmanaged fields for this to test the invariant") + + install(t, liveDocs(t)) + + var asked bool + + gated, gatedStderr, err := runIn(t, bound, Options{ + NonInteractive: true, + ConfirmApply: func(string) (bool, error) { asked = true; return false, nil }, + }) + + require.NoError(t, err) + assert.Empty(t, gated.Plan.Artifact, "the plan under test stays empty of managed changes") + assert.False(t, asked, "unmanaged fields do not arm the gate: nothing would change") + assert.Equal(t, baseline, gatedStderr, "the unmanaged-only output does not drift") +} + +// TestRun_ConfirmFirstDeploy: a first deploy is the loudest thing the gate can +// be asked about, so the create path prompts too, and a decline leaves the +// platform without a workload. +func TestRun_ConfirmFirstDeploy(t *testing.T) { + var created []string + + createSeams := func() fakes { + return fakes{ + create: func(any) (*workload.Workload, error) { + created = append(created, "create") + + return running("wl-new"), nil + }, + wait: func(string, workload.Serving, time.Duration, time.Duration, + func(*workload.Workload), + ) (*workload.Workload, error) { + return running("wl-new"), nil + }, + } + } + + install(t, createSeams()) + + asked := 0 + + _, _, err := runIn(t, unboundImageManifest, Options{ + ConfirmApply: func(string) (bool, error) { asked++; return false, nil }, + }) + + require.ErrorIs(t, err, ErrDeclined) + assert.Equal(t, 1, asked) + assert.Empty(t, created, "a declined first deploy creates nothing") + + install(t, createSeams()) + + result, _, err := runIn(t, unboundImageManifest, Options{ + ConfirmApply: func(string) (bool, error) { asked++; return true, nil }, + }) + + require.NoError(t, err) + assert.Equal(t, 2, asked) + assert.Equal(t, []string{"create"}, created, "accepting proceeds into the create") + assert.Equal(t, "wl-new", result.WorkloadID) +} + +// TestRun_ConfirmWithoutDiff_UsesDefaultRender: --confirm does not require +// --diff. The default plan renders as it always does, then the gate asks; the +// diff renderer stays out of a run that never asked for it. +func TestRun_ConfirmWithoutDiff_UsesDefaultRender(t *testing.T) { + install(t, liveDocs(t)) + + var asked bool + + _, stderr, err := runIn(t, resizedManifest(), Options{ + ConfirmApply: func(string) (bool, error) { asked = true; return false, nil }, + }) + + require.ErrorIs(t, err, ErrDeclined) + assert.True(t, asked) + assert.Contains(t, stderr, "resourceAllocation.memory: 22GB -> 24GB", + "the default plan states a change as have -> want") + assert.NotContains(t, stderr, "\n+ ") + assert.NotContains(t, stderr, "identical lines", "the diff rendering is opt-in and stays opt-in") +} + +// TestRun_ConfirmWithDiff_PromptsAfterTheDiff: the composition renders the +// unified diff, then asks. The diff body is what the user is consenting to, +// so it precedes the question by construction. +func TestRun_ConfirmWithDiff_PromptsAfterTheDiff(t *testing.T) { + install(t, liveDocs(t)) + + var printed int + + var stderr bytes.Buffer + + _, err := runConfirmIn(t, resizedManifest(), Options{ + Diff: true, + ConfirmApply: func(string) (bool, error) { printed = stderr.Len(); return false, nil }, + }, &stderr) + + require.ErrorIs(t, err, ErrDeclined) + + body := stderr.String()[:printed] + + assert.Contains(t, body, "+ containerGroups[default].containers[vllm-server].resourceAllocation.memory: 24GB", + "the diff precedes the question") + assert.NotContains(t, stderr.String()[printed:], "+ ", + "nothing renders past the gate on a decline") +} + +// TestRun_ConfirmAndTypedConfirm_Coexist: the two questions are different +// questions about different things, and they keep their order. The y/N asks +// whether to apply at all; the typed one asks who is rolling production. A +// decline at the y/N never reaches the second question, because there is +// nothing left to roll. +func TestRun_ConfirmAndTypedConfirm_Coexist(t *testing.T) { + var ( + tr track + gateAsk int + typedAsk int + gateFirst bool + ) + + f := lockedLive(wiredRoll(&tr)) + f.guard = func(string) error { + tr.steps = append(tr.steps, "guard") + + return nil + } + + install(t, f) + + // Declining the y/N: the typed question is never asked and nothing runs. + _, _, err := runIn(t, newImage(), Options{ + ConfirmApply: func(string) (bool, error) { gateAsk++; return false, nil }, + Confirm: func(string, string) (bool, error) { + typedAsk++ + + return false, nil + }, + }) + + require.ErrorIs(t, err, ErrDeclined) + assert.Equal(t, 1, gateAsk) + assert.Equal(t, 0, typedAsk, "a declined y/N skips the typed confirm entirely") + assert.NotContains(t, tr.steps, "lock:art-2") + assert.NotContains(t, tr.steps, "replace:art-2") + + // Accepting the y/N hands the run to the typed confirm, which refuses: + // the roll never starts and nothing is locked. + gateAsk, typedAsk = 0, 0 + + _, _, err = runIn(t, newImage(), Options{ + ConfirmApply: func(string) (bool, error) { + gateFirst = typedAsk == 0 + + gateAsk++ + + return true, nil + }, + Confirm: func(string, string) (bool, error) { + typedAsk++ + + return false, nil + }, + }) + + require.Error(t, err) + assert.True(t, gateFirst, "the y/N is asked before the typed confirm") + assert.NotContains(t, tr.steps, "lock:art-2", "a failed typed confirm mutates nothing") + assert.NotContains(t, tr.steps, "replace:art-2") + assert.NotContains(t, tr.steps, "create-artifact") +} diff --git a/internal/workload/up/diff.go b/internal/workload/up/diff.go index 493fe860e..ff0552852 100644 --- a/internal/workload/up/diff.go +++ b/internal/workload/up/diff.go @@ -281,6 +281,169 @@ func join(path, key string) string { return path + "." + key } +// DiffRow is one leaf of want as a unified diff needs it: every leaf the +// file asks for gets a row, changed or not, so a diff can draw the context +// around a change and not just the change. Subset stays the source of truth +// for what differs; DiffRows is Subset's walk with the silent leaves kept. +type DiffRow struct { + // Path is where the field sits, in the file's own spelling, with + // name-keyed lists addressed by name rather than by index. It is also + // the address the plan's redaction hook reads, so a value that must + // never print is refused here on the same terms. + Path string + + // Want is the value the manifest asks for. + Want any + + // Have is the live value, nil when Absent. + Have any + + // Absent distinguishes a field the live object does not carry at all + // from one that carries a different value, exactly as it does for + // Change: adding a probe is not the same act as moving one. + Absent bool + + // Changed reports whether the live side already agrees. Unchanged rows + // are the context a diff draws between changes, and a size written two + // ways is unchanged here for the same reason it is not drift in Subset. + Changed bool +} + +// DiffRows walks every leaf of want, not just the differing ones, marking +// each row changed or unchanged with the same comparison Subset applies: +// name-keyed lists matched by name, memory-equivalent sizes read as equal, +// keys visited in sorted order so two runs over the same inputs read the +// same way. +// +// A leaf the live object does not have is one addition row, a whole element +// at a time, exactly as Subset reports it: adding a container is one act, +// not one per field. With no live side at all (a first deploy) every leaf +// is an addition, walked out to its leaves so the diff can show what will +// be created rather than summarise it. +// +// The unmanaged side is not walked here: what the live object carries that +// the file never names is Extra's question, and the caller merges the two. +func DiffRows(want, have map[string]any) []DiffRow { + var rows []DiffRow + + if have == nil { + additionRows("", want, &rows) + + return rows + } + + walkRows("", want, have, true, &rows) + + return rows +} + +// walkRows compares one node, the way walk does, but answers with a row for +// every leaf rather than only the disagreeing ones. present says whether +// have was actually there, so a key holding an explicit null is not confused +// with a key that is missing. +func walkRows(path string, want, have any, present bool, out *[]DiffRow) { + if !present { + *out = append(*out, DiffRow{Path: path, Want: want, Absent: true, Changed: true}) + + return + } + + switch w := want.(type) { + case map[string]any: + walkRowsMap(path, w, have, out) + case []any: + walkRowsList(path, w, have, out) + default: + *out = append(*out, DiffRow{Path: path, Want: want, Have: have, Changed: !equalAt(path, want, have)}) + } +} + +// walkRowsMap recurses into an object, in key order so the rows read the +// same way twice. A have side that is not an object is one changed row: it +// cannot be matched key by key, and guessing at a comparison would report +// less than the truth. +func walkRowsMap(path string, want map[string]any, have any, out *[]DiffRow) { + h, ok := have.(map[string]any) + if !ok { + *out = append(*out, DiffRow{Path: path, Want: want, Have: have, Changed: true}) + + return + } + + for _, key := range slices.Sorted(maps.Keys(want)) { + hv, present := h[key] + + walkRows(join(path, key), want[key], hv, present, out) + } +} + +// walkRowsList compares a list under the same two shapes walkList knows: +// containerGroups, containers and environmentVars are keyed by name and +// matched by it, so a reordered live object moves no row, and everything +// else, a resourceBundles of plain strings or an autoscaling policy with no +// name to key on, is one leaf compared whole. +func walkRowsList(path string, want []any, have any, out *[]DiffRow) { + h, ok := have.([]any) + if !ok { + *out = append(*out, DiffRow{Path: path, Want: want, Have: have, Changed: true}) + + return + } + + if !nameKeyed(want) { + *out = append(*out, DiffRow{Path: path, Want: want, Have: h, Changed: !equal(want, h)}) + + return + } + + live := byName(h) + + for _, item := range want { + element, _ := item.(map[string]any) + name, _ := element["name"].(string) + at := fmt.Sprintf("%s[%s]", path, name) + + counterpart, found := live[name] + if !found { + *out = append(*out, DiffRow{Path: at, Want: element, Absent: true, Changed: true}) + + continue + } + + walkRowsMap(at, element, counterpart, out) + } +} + +// additionRows walks want with no live side to compare against, as a first +// deploy is: every leaf becomes an addition. Objects and name-keyed lists +// are walked through to their leaves, so the diff can show what will be +// created field by field; anything without a name to walk into, an unkeyed +// list or a scalar, is one row as it stands. +func additionRows(path string, want any, out *[]DiffRow) { + switch w := want.(type) { + case map[string]any: + for _, key := range slices.Sorted(maps.Keys(w)) { + additionRows(join(path, key), w[key], out) + } + case []any: + if !nameKeyed(w) { + *out = append(*out, DiffRow{Path: path, Want: w, Absent: true, Changed: true}) + + return + } + + for _, item := range w { + element, _ := item.(map[string]any) + name, _ := element["name"].(string) + at := fmt.Sprintf("%s[%s]", path, name) + + additionRows(at, element, out) + } + default: + *out = append(*out, DiffRow{Path: path, Want: want, Absent: true, Changed: true}) + } +} + // format renders a value for the plan. Composite values are summarised // rather than dumped: a plan is a summary, and a reader who wants the whole // object has the file open next to it. diff --git a/internal/workload/up/diff_test.go b/internal/workload/up/diff_test.go index 372a429c9..490f2cd90 100644 --- a/internal/workload/up/diff_test.go +++ b/internal/workload/up/diff_test.go @@ -481,3 +481,414 @@ func TestSubset_SiblingKeysDoNotShareStorage(t *testing.T) { assert.Equal(t, map[string]bool{"port": true, "cpu": true}, last, "each sibling keeps its own last segment") } + +// rowPaths is the set of walked paths, which is what most DiffRows cases +// care about. +func rowPaths(rows []DiffRow) []string { + out := make([]string, 0, len(rows)) + for _, r := range rows { + out = append(out, r.Path) + } + + return out +} + +// changedRows keeps the rows the two sides disagree about, which is exactly +// the set Subset reports. DiffRows is Subset's walk with the agreeing leaves +// kept, so the two must answer the same question with the same records. +func changedRows(rows []DiffRow) []DiffRow { + out := make([]DiffRow, 0, len(rows)) + for _, r := range rows { + if r.Changed { + out = append(out, r) + } + } + + return out +} + +// assertSameFindings pins the classification to Subset's: for the same two +// sides, the changed rows are the changes, one for one, values and all. A +// diff that disagreed with the plan about what differs would be worse than +// no diff at all. +func assertSameFindings(t *testing.T, want, have map[string]any) { + t.Helper() + + changes := Subset(want, have) + rows := changedRows(DiffRows(want, have)) + + require.Len(t, rows, len(changes)) + + for i, c := range changes { + assert.Equal(t, c.Path, rows[i].Path, "row %d path", i) + assert.Equal(t, c.Want, rows[i].Want, "row %d want", i) + assert.Equal(t, c.Have, rows[i].Have, "row %d have", i) + assert.Equal(t, c.Absent, rows[i].Absent, "row %d absent", i) + } +} + +// TestDiffRows_EmitsOneRowPerLeafChangedOrUnchanged is the difference from +// Subset: a diff needs the leaves that already agree as context, so every +// leaf of want gets a row, in sorted order, marked. +func TestDiffRows_EmitsOneRowPerLeafChangedOrUnchanged(t *testing.T) { + rows := DiffRows( + obj(t, `{"type":"service","port":9090,"probe":{"path":"/health","port":8000}}`), + obj(t, `{"type":"service","port":8080,"probe":{"path":"/health","port":8000}}`), + ) + + require.Len(t, rows, 4, "one row per leaf of want, not one per difference") + assert.Equal(t, []string{"port", "probe.path", "probe.port", "type"}, rowPaths(rows)) + + changed := rows[0] + + assert.True(t, changed.Changed) + assert.False(t, changed.Absent) + assert.InDelta(t, 9090.0, changed.Want, 0) + assert.InDelta(t, 8080.0, changed.Have, 0) + + for _, r := range rows[1:] { + assert.False(t, r.Changed, "leaf %s agrees, so it is context", r.Path) + } +} + +// TestDiffRows_ChangedRowsAgreeWithSubset runs the fixtures Subset's own +// tests use through both walkers: whatever Subset reports, DiffRows must +// mark changed, with the same path, values and absent flag. +func TestDiffRows_ChangedRowsAgreeWithSubset(t *testing.T) { + tests := []struct { + name string + want string + have string + }{ + { + name: "a moved scalar", + want: `{"port":9090}`, + have: `{"port":8080}`, + }, + { + name: "a field the live object lacks", + want: `{"readinessProbe":{"path":"/health"}}`, + have: `{}`, + }, + { + name: "an explicit null on the live side", + want: `{"entrypoint":["uvicorn"]}`, + have: `{"entrypoint":null}`, + }, + { + name: "a port inside a name-keyed list", + want: `{"containerGroups":[{"name":"default","containers":[{"name":"primary","port":9090}]}]}`, + have: `{"containerGroups":[{"name":"default","containers":[{"name":"primary","port":8080}]}]}`, + }, + { + name: "a whole element the live object lacks", + want: `{"containers":[{"name":"primary","port":8080},{"name":"sidecar","port":9000}]}`, + have: `{"containers":[{"name":"primary","port":8080}]}`, + }, + { + name: "an unkeyed list that differs", + want: `{"resourceBundles":["gpu.large"]}`, + have: `{"resourceBundles":["gpu.medium"]}`, + }, + { + name: "a type mismatch", + want: `{"probe":{"path":"/health"}}`, + have: `{"probe":"/health"}`, + }, + { + name: "a genuinely different memory size", + want: `{"resourceAllocation":{"memory":"1GB"}}`, + have: `{"resourceAllocation":{"memory":512000000.0}}`, + }, + { + name: "a rotated credential reference", + want: `{"containers":[{"name":"vllm-server","environmentVars":[ + {"name":"HUGGING_FACE_HUB_TOKEN","source":"dr-credential", + "drCredentialId":"aaaaaaaaaaaaaaaaaaaaaaaa","key":"apiToken"}]}]}`, + have: `{"containers":[{"name":"vllm-server","environmentVars":[ + {"name":"HF_HOME","value":"/tmp/hf"}, + {"name":"HUGGING_FACE_HUB_TOKEN","source":"dr-credential", + "drCredentialId":"66f1a2b3c4d5e6f7a8b9c0d1","key":"apiToken"}]}]}`, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + assertSameFindings(t, obj(t, tc.want), obj(t, tc.have)) + }) + } + + // And the real spec, where the file moved two things and addressed them + // by name rather than by position. + file := obj(t, `{ + "type": "service", + "containerGroups": [ + { + "name": "default", + "containers": [ + { + "name": "vllm-server", + "port": 8080, + "readinessProbe": {"path": "/ready", "port": 8000} + } + ] + } + ] + }`) + + assertSameFindings(t, file, obj(t, liveSpecJSON)) + + // A file that says less than the live object: every leaf it does name + // agrees, so the diff is all context and Subset is silent. + quiet := obj(t, `{ + "type": "service", + "containerGroups": [ + { + "name": "default", + "containers": [ + { + "name": "vllm-server", + "primary": true, + "port": 8000, + "imageBuildConfig": {"dockerfile": {"source": "provided"}}, + "readinessProbe": {"path": "/health", "port": 8000} + } + ] + } + ] + }`) + + assert.Empty(t, Subset(quiet, obj(t, liveSpecJSON))) + assert.Empty(t, changedRows(DiffRows(quiet, obj(t, liveSpecJSON))), + "nothing the file names differs, so the diff has no changes to show") +} + +// TestDiffRows_NameKeyedListsMatchByNameNotIndex is why the walk matches by +// name: the platform returns container groups in whatever order it likes, +// and a reordered live object must move no row and mark none changed. +func TestDiffRows_NameKeyedListsMatchByNameNotIndex(t *testing.T) { + want := obj(t, `{"containers":[ + {"name":"primary","port":8080}, + {"name":"sidecar","port":9000} + ]}`) + have := obj(t, `{"containers":[ + {"name":"sidecar","port":9000}, + {"name":"primary","port":8080} + ]}`) + sorted := obj(t, `{"containers":[ + {"name":"primary","port":8080}, + {"name":"sidecar","port":9000} + ]}`) + + reordered := DiffRows(want, have) + + assert.Equal(t, DiffRows(want, sorted), reordered, + "the live order of a name-keyed list moves no row") + + for _, r := range reordered { + assert.False(t, r.Changed, "reordering a name-keyed list is not drift") + } +} + +func TestDiffRows_NameKeyedPathUsesTheName(t *testing.T) { + rows := DiffRows( + obj(t, `{"containerGroups":[{"name":"default","containers":[{"name":"primary","port":9090}]}]}`), + obj(t, `{"containerGroups":[{"name":"default","containers":[{"name":"primary","port":8080}]}]}`), + ) + + assert.Equal(t, []string{ + "containerGroups[default].containers[primary].name", + "containerGroups[default].containers[primary].port", + "containerGroups[default].name", + }, rowPaths(rows)) + + changed := changedRows(rows) + + require.Len(t, changed, 1) + assert.Equal(t, "containerGroups[default].containers[primary].port", changed[0].Path) +} + +func TestDiffRows_NameKeyedElementMissingLiveIsOneAddition(t *testing.T) { + rows := DiffRows( + obj(t, `{"containers":[{"name":"primary","port":8080},{"name":"sidecar","port":9000}]}`), + obj(t, `{"containers":[{"name":"primary","port":8080}]}`), + ) + + assert.Equal(t, []string{ + "containers[primary].name", + "containers[primary].port", + "containers[sidecar]", + }, rowPaths(rows)) + + additions := changedRows(rows) + + require.Len(t, additions, 1, "a whole new container is one addition, not one per field") + assert.Equal(t, "containers[sidecar]", additions[0].Path) + assert.True(t, additions[0].Absent) +} + +// TestDiffRows_UnkeyedListsCompareWhole covers resourceBundles (plain +// strings) and autoscaling policies (objects with no name). Neither has +// anything to key on, so each list is one leaf and a partial match would +// mean nothing. +func TestDiffRows_UnkeyedListsCompareWhole(t *testing.T) { + unchanged := DiffRows( + obj(t, `{"resourceBundles":["gpu.medium"]}`), + obj(t, `{"resourceBundles":["gpu.medium"]}`), + ) + require.Len(t, unchanged, 1) + assert.False(t, unchanged[0].Changed) + + changed := DiffRows( + obj(t, `{"resourceBundles":["gpu.large"]}`), + obj(t, `{"resourceBundles":["gpu.medium"]}`), + ) + require.Len(t, changed, 1) + assert.True(t, changed[0].Changed) + + policies := DiffRows( + obj(t, `{"policies":[{"scalingMetric":"gpuCacheUtilization","target":80}]}`), + obj(t, `{"policies":[{"scalingMetric":"gpuCacheUtilization","target":70}]}`), + ) + require.Len(t, policies, 1, "an object with no name to key on is one leaf too") + assert.True(t, policies[0].Changed) +} + +func TestDiffRows_EmptyListsAreCompatible(t *testing.T) { + bothEmpty := DiffRows(obj(t, `{"containers":[]}`), obj(t, `{"containers":[]}`)) + require.Len(t, bothEmpty, 1) + assert.False(t, bothEmpty[0].Changed) + + liveHasMore := DiffRows(obj(t, `{"containers":[]}`), obj(t, `{"containers":[{"name":"primary"}]}`)) + require.Len(t, liveHasMore, 1, "an empty list has nothing to key on, so it is compared whole") + assert.True(t, liveHasMore[0].Changed) +} + +func TestDiffRows_TypeMismatchIsOneChange(t *testing.T) { + rows := DiffRows( + obj(t, `{"probe":{"path":"/health"}}`), + obj(t, `{"probe":"/health"}`), + ) + + require.Len(t, rows, 1, "an object asked of a scalar is one row, not one per field") + assert.Equal(t, "probe", rows[0].Path) + assert.True(t, rows[0].Changed) + assert.False(t, rows[0].Absent) + + listVsScalar := DiffRows( + obj(t, `{"entrypoint":["uvicorn","app:app"]}`), + obj(t, `{"entrypoint":"uvicorn app:app"}`), + ) + require.Len(t, listVsScalar, 1) + assert.True(t, listVsScalar[0].Changed) +} + +// TestDiffRows_ExplicitNullLiveIsAChangeNotAnAddition separates "the key is +// there holding null" from "the key is not there", which the walk tracks +// with its present flag. Both render differently in a diff. +func TestDiffRows_ExplicitNullLiveIsAChangeNotAnAddition(t *testing.T) { + rows := DiffRows( + obj(t, `{"entrypoint":["uvicorn"]}`), + obj(t, `{"entrypoint":null}`), + ) + + require.Len(t, rows, 1) + assert.False(t, rows[0].Absent, "the key is there holding null, which is not the same as missing") + assert.Nil(t, rows[0].Have) + assert.True(t, rows[0].Changed) +} + +func TestDiffRows_EmptyWantAsksForNothing(t *testing.T) { + assert.Empty(t, DiffRows(obj(t, `{}`), obj(t, `{"anything":1}`))) + assert.Empty(t, DiffRows(obj(t, `{"probe":{}}`), obj(t, `{"probe":{"path":"/health"}}`)), + "an object the file leaves empty has no leaves to walk") +} + +// TestDiffRows_MemoryIsTheSameSizeWrittenTwoWays is the diff-mode half of +// the memory tolerance: "512MB" in the file and 512000000 in the live +// workload are one sizing written twice, so the leaf reads as context and +// not as a change no deploy would settle. Reporting it as a change would +// mean every run showed permanent drift in a diff that never shrinks. +func TestDiffRows_MemoryIsTheSameSizeWrittenTwoWays(t *testing.T) { + fileString := DiffRows( + obj(t, `{"resourceAllocation":{"memory":"512MB"}}`), + obj(t, `{"resourceAllocation":{"memory":512000000.0}}`), + ) + require.Len(t, fileString, 1) + assert.False(t, fileString[0].Changed, "512MB and 512000000 are the same sizing") + + fileNumber := DiffRows( + obj(t, `{"resourceAllocation":{"memory":512000000.0}}`), + obj(t, `{"resourceAllocation":{"memory":"512MB"}}`), + ) + require.Len(t, fileNumber, 1) + assert.False(t, fileNumber[0].Changed, "the tolerance does not care which side spells it") +} + +func TestDiffRows_ADifferentMemoryIsStillDrift(t *testing.T) { + // 536870912 is 512MiB, a binary size: the platform reads 512MB as + // 512000000 bytes, so the two are genuinely different sizings. + rows := DiffRows( + obj(t, `{"resourceAllocation":{"memory":"512MB"}}`), + obj(t, `{"resourceAllocation":{"memory":536870912.0}}`), + ) + require.Len(t, rows, 1) + assert.True(t, rows[0].Changed, "512MB is not 536870912 bytes") + + spelled := DiffRows( + obj(t, `{"resourceAllocation":{"memory":"512MB"}}`), + obj(t, `{"resourceAllocation":{"memory":"1GB"}}`), + ) + require.Len(t, spelled, 1) + assert.True(t, spelled[0].Changed, "512MB is not 1GB") +} + +// TestDiffRows_NilHaveMeansEveryLeafIsAnAddition is the first deploy: there +// is no live object to agree with, so every leaf of want is an addition, +// walked out to its leaves so a diff can show what will be created rather +// than summarise it. +func TestDiffRows_NilHaveMeansEveryLeafIsAnAddition(t *testing.T) { + rows := DiffRows( + obj(t, `{"type":"service","port":8080,"probe":{"path":"/health"}, + "containers":[{"name":"primary","port":8080}], + "resourceBundles":["gpu.medium"]}`), + nil, + ) + + assert.Equal(t, []string{ + "containers[primary].name", + "containers[primary].port", + "port", + "probe.path", + "resourceBundles", + "type", + }, rowPaths(rows)) + + for _, r := range rows { + assert.True(t, r.Absent, "leaf %s has nothing live to agree with", r.Path) + assert.True(t, r.Changed, "leaf %s is an addition on a first deploy", r.Path) + assert.Nil(t, r.Have) + } +} + +// TestDiffRows_OrderIsStable keeps a diff from reading differently on two +// runs over the same inputs, which would make a diffed CI log useless. The +// inputs are parsed fresh each round: Go randomises map iteration order, and +// the rows must not care. +func TestDiffRows_OrderIsStable(t *testing.T) { + const ( + wantJSON = `{"zebra":1,"alpha":{"m":2,"a":3}, + "containerGroups":[{"name":"default","containers":[{"name":"primary","port":9090}]}]}` + + haveJSON = `{"zebra":0,"alpha":{"m":0,"a":0}, + "containerGroups":[{"name":"default","containers":[{"name":"primary","port":8080}]}]}` + ) + + first := DiffRows(obj(t, wantJSON), obj(t, haveJSON)) + require.NotEmpty(t, first) + + for range 25 { + assert.Equal(t, first, DiffRows(obj(t, wantJSON), obj(t, haveJSON)), + "the same inputs must read the same way twice") + } +} diff --git a/internal/workload/up/plan.go b/internal/workload/up/plan.go index ba7ec9126..aa076ac40 100644 --- a/internal/workload/up/plan.go +++ b/internal/workload/up/plan.go @@ -18,6 +18,7 @@ import ( "slices" "github.com/datarobot/cli/internal/workload/manifest" + "github.com/datarobot/cli/internal/workload/sync" ) // Actions are what a run will do, and what the JSON envelope reports. A run @@ -64,6 +65,15 @@ type CodeChange struct { // the link onto it, which the plan says out loud because nothing in the // file asked for it. LinkLocked bool + + // SyncPlan is the dry-run plan the sync engine measured this tree with, + // the same one Files counts. --diff renders it through the sync command's + // own plan printer, so the two commands describe an upload with one + // format instead of two. It is plain data rather than engine state -- the + // lock is released right after measuring -- so carrying it past the + // engine's Close costs nothing. Nil on a first deploy, for a manifest + // that names an image, and when a test harness wires only the count. + SyncPlan *sync.SyncPlan } // Changed reports whether the code needs syncing and rebuilding. @@ -104,6 +114,22 @@ type Plan struct { // InheritsImage reports that the new version can take the running image. // What the plan intends, not a promise; the envelope is corrected after. InheritsImage bool + // DiffArtifact and DiffRuntime are the same two halves for the --diff + // rendering: every leaf the file names, changed or not, so a diff can + // draw context around its changes instead of listing only what moves. + // They come from the walks that produce Artifact and Runtime, so the two + // renderings of one plan cannot disagree about what differs. The changes + // below that no walk can produce -- the artifact id and type -- are + // merged into the rows as well, because a change the default plan prints + // must never be invisible in the diff. + DiffArtifact []DiffRow + DiffRuntime []DiffRow + + // Unmanaged lists the name-keyed elements the live object carries that + // the file never names, from Extra. The diff renders them as a count + // rather than as the removals they are not: the file leaving a field out + // is the file declining to manage it, not asking for it to be deleted. + Unmanaged []string // Locked reports that the version now serving is immutable. Its successor // has to be locked too before the platform will take it, so a deploy onto @@ -344,6 +370,16 @@ func Build(loaded Loaded, live Live, code CodeChange, opts Options) (Plan, error // Nothing exists to compare against, so every field is trivially an // addition. Saying so once is a plan; saying it per field is a wall. if plan.Creates { + artifactRows, runtimeRows, err := createRows(loaded) + if err != nil { + return Plan{}, err + } + + // The diff has no live side to draw context from either, so it walks + // the file against nil and shows what will be created, leaf by leaf. + plan.DiffArtifact = artifactRows + plan.DiffRuntime = runtimeRows + return plan, nil } @@ -360,6 +396,20 @@ func Build(loaded Loaded, live Live, code CodeChange, opts Options) (Plan, error plan.Artifact = Subset(spec, live.Spec) plan.Runtime = Subset(runtime, live.Runtime) + // The diff rows are the same two walks with the agreeing leaves kept, so + // the changes above and the context around them come from one comparison + // and cannot disagree about what differs. + plan.DiffArtifact = DiffRows(spec, live.Spec) + plan.DiffRuntime = DiffRows(runtime, live.Runtime) + + // A diff that stayed silent about what the live object carries that the + // file never names would read as an exhaustive account of the workload, + // so the paths land on the plan for the renderer to count. The two + // documents can hold the same unmanaged element -- a sidecar exists in + // the artifact's spec and in the workload's runtime -- and one element is + // one field to a reader, so the list holds each path once. + plan.Unmanaged = dedupePaths(Extra(spec, live.Spec), Extra(runtime, live.Runtime)) + // A file that names an artifact by id describes no spec to compare, so // the walk above has nothing to say about it. Pointing at a different // version than the one running is still the whole plan: it is a roll, and @@ -372,6 +422,13 @@ func Build(loaded Loaded, live Live, code CodeChange, opts Options) (Plan, error Have: live.ArtifactID, Want: bound, }) + + plan.DiffArtifact = append(plan.DiffArtifact, DiffRow{ + Path: keyArtifactID, + Have: live.ArtifactID, + Want: bound, + Changed: true, + }) } kind, err := loaded.ArtifactType() @@ -412,6 +469,13 @@ func Build(loaded Loaded, live Live, code CodeChange, opts Options) (Plan, error Have: running, Want: kind, }) + + plan.DiffArtifact = append(plan.DiffArtifact, DiffRow{ + Path: keyArtifactType, + Have: running, + Want: kind, + Changed: true, + }) } // Last: every drift has to be in hand before RebuildsImage can answer. @@ -419,3 +483,53 @@ func Build(loaded Loaded, live Live, code CodeChange, opts Options) (Plan, error return plan, nil } + +// createRows walks the file against nothing, which is what a create compares +// against: the spec half and the runtime half both come back as additions, +// one row per leaf, so a diff can show what will be created rather than +// summarise it. +func createRows(loaded Loaded) ([]DiffRow, []DiffRow, error) { + spec, err := loaded.Spec() + if err != nil { + return nil, nil, err + } + + runtime, err := loaded.Runtime() + if err != nil { + return nil, nil, err + } + + return DiffRows(spec, nil), DiffRows(runtime, nil), nil +} + +// dedupePaths merges the unmanaged path lists from the two halves of the +// plan, keeping the first sighting of each path. The lists each arrive in a +// deterministic order, and the same element can be reported by both -- a +// sidecar the file never names exists in the artifact's spec and in the +// workload's runtime alike -- so counting it twice would promise a reader +// two fields where there is one name to go look at. +func dedupePaths(lists ...[]string) []string { + var total int + + for _, list := range lists { + total += len(list) + } + + seen := make(map[string]struct{}, total) + + out := make([]string, 0, total) + + for _, list := range lists { + for _, path := range list { + if _, ok := seen[path]; ok { + continue + } + + seen[path] = struct{}{} + + out = append(out, path) + } + } + + return out +} diff --git a/internal/workload/up/plan_test.go b/internal/workload/up/plan_test.go index 11033becf..65d0530df 100644 --- a/internal/workload/up/plan_test.go +++ b/internal/workload/up/plan_test.go @@ -762,3 +762,151 @@ func allStates() []State { StateErrored, } } + +// TestBuild_DiffRowsMirrorTheArtifactRuntimeSplit: the diff rows come from +// the same two walks the change lists do, split the same way, so a diff and +// the plan beside it can never disagree about what differs or which block a +// finding belongs to. The agreeing leaves are kept as context, which is what +// makes the rows a diff rather than a second change list. +func TestBuild_DiffRowsMirrorTheArtifactRuntimeSplit(t *testing.T) { + drifted := `{ + "name": "my-app", + "artifact": {"name": "my-app-artifact", "spec": {"containerGroups": [ + {"name": "default", "containers": [{"name": "primary", "port": 9090}]} + ]}}, + "runtime": {"containerGroups": [{"name": "default", "replicaCount": 3}]} + }` + + plan, err := Build( + loadedFrom(drifted), + liveFrom(t, StateRunning, planLiveSpec, planLiveRuntime), + builtCode(0), + Options{}, + ) + require.NoError(t, err) + + assert.Equal(t, paths(plan.Artifact), rowPaths(changedRows(plan.DiffArtifact))) + assert.Equal(t, paths(plan.Runtime), rowPaths(changedRows(plan.DiffRuntime))) + + require.Greater(t, len(plan.DiffArtifact), len(plan.Artifact), "the spec rows keep the agreeing leaves") + + var port DiffRow + + for _, r := range plan.DiffArtifact { + if r.Path == "containerGroups[default].containers[primary].port" && !r.Changed { + port = r + } + } + + assert.False(t, port.Changed, "a leaf the file and the workload agree on is context") +} + +// TestBuild_CreatePathComputesAllAdditionRows: a create has no live side to +// compare against, so the diff walks the file against nil and every leaf is +// an addition, walked out per leaf. The default plan's halves stay empty, +// because a create says so once rather than per field. +func TestBuild_CreatePathComputesAllAdditionRows(t *testing.T) { + plan, err := Build( + loadedFrom(planPayload), + Live{State: StateUnbound}, + CodeChange{Applies: true, FirstDeploy: true}, + Options{}, + ) + require.NoError(t, err) + + require.NotEmpty(t, plan.DiffArtifact) + + for _, r := range plan.DiffArtifact { + assert.True(t, r.Absent, "row %s", r.Path) + assert.True(t, r.Changed, "row %s", r.Path) + assert.Nil(t, r.Have, "row %s", r.Path) + } + + require.NotEmpty(t, plan.DiffRuntime) + + for _, r := range plan.DiffRuntime { + assert.True(t, r.Absent, "row %s", r.Path) + assert.True(t, r.Changed, "row %s", r.Path) + } + + assert.Empty(t, plan.Unmanaged, "nothing is live, so nothing is unmanaged") + assert.Empty(t, plan.Artifact) + assert.Empty(t, plan.Runtime) +} + +// TestBuild_UnmanagedComesFromExtra: what the live object carries that the +// file never names lands on the plan as paths, so the diff can count it. +// The same element is unmanaged in both documents here -- the metrics +// sidecar exists in the artifact's spec and in the workload's runtime -- and +// one element is one field to a reader, so the list holds the path once. +func TestBuild_UnmanagedComesFromExtra(t *testing.T) { + plan, err := Build( + loadedFrom(planPayload), + liveFrom(t, StateRunning, planLiveSpec, planLiveRuntime), + builtCode(0), + Options{}, + ) + require.NoError(t, err) + + assert.Equal(t, + []string{"containerGroups[default].containers[metrics]"}, + plan.Unmanaged) +} + +// TestBuild_SyntheticArtifactIDChangeEntersTheDiffRows: the artifact id is +// not a leaf of the spec, so no walk of it can produce the change. It is +// merged into the rows all the same, because a change the default plan +// prints must not be invisible in the diff. +func TestBuild_SyntheticArtifactIDChangeEntersTheDiffRows(t *testing.T) { + loaded := Loaded{Compiled: &manifest.Compiled{ + Payload: json.RawMessage(`{"name": "my-app", "artifactId": "68b0bbbb0000000000000002"}`), + ArtifactID: "68b0bbbb0000000000000002", + }} + + live := liveFrom(t, StateRunning, "", planLiveRuntime) + live.ArtifactID = "68a0000000000000000000a1" + + plan, err := Build(loaded, live, builtCode(0), Options{}) + require.NoError(t, err) + + changed := changedRows(plan.DiffArtifact) + + require.Len(t, changed, 1) + assert.Equal(t, "artifactId", changed[0].Path) + assert.Equal(t, "68a0000000000000000000a1", changed[0].Have) + assert.Equal(t, "68b0bbbb0000000000000002", changed[0].Want) + + out := renderDiff(t, appSummary, plan) + + assert.Contains(t, out, "- artifactId: 68a0000000000000000000a1") + assert.Contains(t, out, "+ artifactId: 68b0bbbb0000000000000002") +} + +// The type sits beside the spec for the same reason, and lands in the rows +// the same way, with the defaulted live value on the left of the pair. +func TestBuild_SyntheticArtifactTypeChangeEntersTheDiffRows(t *testing.T) { + loaded := Loaded{Compiled: &manifest.Compiled{ + Payload: json.RawMessage(`{ + "name": "my-app", + "artifact": {"name": "my-app-artifact", "type": "agent", "spec": {}} + }`), + }} + + live := liveFrom(t, StateRunning, "", planLiveRuntime) + live.ArtifactType = "service" + + plan, err := Build(loaded, live, builtCode(0), Options{}) + require.NoError(t, err) + + changed := changedRows(plan.DiffArtifact) + + require.Len(t, changed, 1) + assert.Equal(t, "artifact.type", changed[0].Path) + assert.Equal(t, "service", changed[0].Have) + assert.Equal(t, "agent", changed[0].Want) + + out := renderDiff(t, appSummary, plan) + + assert.Contains(t, out, "- artifact.type: service") + assert.Contains(t, out, "+ artifact.type: agent") +} diff --git a/internal/workload/up/render.go b/internal/workload/up/render.go index 3dadfaa6d..b543b1776 100644 --- a/internal/workload/up/render.go +++ b/internal/workload/up/render.go @@ -20,8 +20,10 @@ import ( "strings" "github.com/charmbracelet/lipgloss" + "github.com/datarobot/cli/internal/uidiff" "github.com/datarobot/cli/internal/workload" "github.com/datarobot/cli/internal/workload/manifest" + "github.com/datarobot/cli/internal/workload/sync/display" "github.com/datarobot/cli/tui" ) @@ -55,7 +57,16 @@ const detailLimit = 6 // environment variable can be a secret someone pasted in plaintext, and a // plan that echoed it would put it in terminal scrollback and CI logs. Names // are enough to see what changed. -const envVarsSegment = ".environmentVars[" +const ( + // envVarsKey is the block's own key in a decoded manifest, the one the + // subtree scrub looks for: a whole-element row's path names the element + // around it, so catching secrets below it means reading the value, not + // the path. + envVarsKey = "environmentVars" + + // envVarsSegment is how envVarsKey reads inside a leaf's path. + envVarsSegment = "." + envVarsKey + "[" +) // Render writes the plan block that `up` prints before it acts, and that // --dry-run prints instead of acting. @@ -87,6 +98,215 @@ func Render(w io.Writer, s Summary, plan Plan) error { return err } +// RenderDiff writes the plan block as a unified diff, for `up --diff`. +// +// Where Render summarises, this lays the plan out leaf by leaf: a changed +// field states both sides of itself, `- old` then `+ new`; an agreeing field +// is context that collapses once it runs past the window; and what the live +// object carries that the file never names is counted once, because none of +// it is a removal. The default plan caps its detail list because a plan is a +// summary, and the file is a better place to read the rest; a diff is the +// detail, so nothing here is capped. +func RenderDiff(w io.Writer, s Summary, plan Plan) error { + var b strings.Builder + + if head := diffHeader(s, plan); head != "" { + b.WriteString(planTitleStyle.Render(head)) + b.WriteString("\n") + } + + if plan.Empty() { + // The three-state rule is the point of --diff, so an empty plan does + // not skip the block the way Render's does: what the live object + // carries that the file never names still gets said. The verdict is + // the line the default mode prints, because the run's outcome, and + // its exit code, are the same run's. + b.WriteString("\n" + settledVerdict(plan) + "\n") + + if note := unmanagedNote(plan.Unmanaged); note != "" { + b.WriteString(note) + b.WriteString("\n") + } + + _, err := io.WriteString(w, b.String()) + + return err + } + + b.WriteString("\n") + + actions, err := diffActionLines(s, plan) + if err != nil { + return err + } + + for _, line := range actions { + b.WriteString(line) + b.WriteString("\n") + } + + if err := uidiff.Render(&b, diffRows(plan), uidiff.Options{Redact: redacted}); err != nil { + return err + } + + if note := unmanagedNote(plan.Unmanaged); note != "" { + b.WriteString(note) + b.WriteString("\n") + } + + _, err = io.WriteString(w, b.String()) + + return err +} + +// diffActionLines are the lines a diff keeps from the default plan: the +// reasons the run will act. None of them is a field change, so each keeps +// the entry form and position the default plan gives it -- rendered as a +// hunk, a stopped workload or a locked version would read as an edit to a +// field nobody wrote. +func diffActionLines(s Summary, plan Plan) ([]string, error) { + var out []string + + if plan.Creates { + out = append(out, entry("+", "workload", createDetail(s, plan))) + } + + if line := stateLine(plan.State, s.Status); line != "" { + out = append(out, line) + } + + code, err := codeBlock(plan.Code) + if err != nil { + return nil, err + } + + out = append(out, code...) + out = append(out, lockLines(plan)...) + out = append(out, artifactEntry(plan)...) + + return out, nil +} + +// codeBlock is the diff's code section, and it exists only for code the +// deploy will push: an entry saying nothing moved, or a file list for a sync +// the run will not perform, would both invent work. The gate is Changed(), +// one predicate for the whole block, because Files counts exactly the plan's +// Uploads and Deletes -- the rows a deploy pushes. IsEmpty() would also count +// Downloads and Conflicts, which the deploy never pulls, and would then print +// a file list for a tree the run leaves alone. A first deploy has no sync +// plan to list, because nothing was ever uploaded to compare the tree +// against, so it keeps the default plan's wording. Otherwise the section is +// the file list the sync would upload, drawn by the sync command's own plan +// printer and fed the dry-run plan the code-change seam carried: two formats +// for one upload would eventually disagree, which is the whole reason the +// list is borrowed rather than redrawn. A plan measured without its list -- +// no production path makes one, but a harness wiring only the count does -- +// falls back to the count rather than printing nothing about real drift. +func codeBlock(code CodeChange) ([]string, error) { + if !code.Changed() { + return nil, nil + } + + if code.FirstDeploy || code.SyncPlan == nil || len(code.SyncPlan.Uploads)+len(code.SyncPlan.Deletes) == 0 { + return []string{entry("~", "code", codeDetail(code))}, nil + } + + var b strings.Builder + + if err := display.PrintPlan(&b, code.SyncPlan); err != nil { + return nil, fmt.Errorf("render the sync file list: %w", err) + } + + return strings.Split(strings.TrimSuffix(b.String(), "\n"), "\n"), nil +} + +// diffHeader names the plan's subject. A live workload is named exactly as +// the default plan names it. A first deploy has no live side to name, and +// above a diff of the file itself the default plan's silence would leave the +// block unheaded, so the header states the subject instead. A create that +// replaces a dead binding is not a first deploy and still gets no header: +// the create line is the warning, as it is in the default mode. +func diffHeader(s Summary, plan Plan) string { + if !plan.Creates { + return header(s, plan) + } + + if plan.PriorWorkloadID != "" { + return "" + } + + name := s.Name + if name == "" { + name = "workload" + } + + return name + ", first deploy" +} + +// diffRows flattens the plan's two row halves into the order the diff draws +// them, the artifact spec first and the runtime sizing second, one diff +// either way: they are one deploy and the reader reviews them together. +func diffRows(plan Plan) []uidiff.Row { + rows := make([]uidiff.Row, 0, len(plan.DiffArtifact)+len(plan.DiffRuntime)) + + rows = appendLeafRows(rows, plan.DiffArtifact) + rows = appendLeafRows(rows, plan.DiffRuntime) + + return rows +} + +// appendLeafRows turns whole-leaf rows into diff lines. A changed leaf +// becomes two lines, the value it replaces and the value it becomes, because +// that is what a unified diff states; an addition is one line, and an +// agreeing leaf is context. Redaction is left to the uidiff hook, which +// rewrites the whole line for a path it refuses, so a value formatted into +// the text here is never what prints. +func appendLeafRows(rows []uidiff.Row, leaves []DiffRow) []uidiff.Row { + for _, leaf := range leaves { + switch { + case !leaf.Changed: + rows = append(rows, uidiff.Row{ + Kind: uidiff.Context, + Path: leaf.Path, + Text: leafText(leaf.Path, leaf.Want), + }) + case leaf.Absent: + rows = append(rows, uidiff.Row{ + Kind: uidiff.Add, + Path: leaf.Path, + Text: leafText(leaf.Path, leaf.Want), + }) + default: + rows = append(rows, + uidiff.Row{Kind: uidiff.Del, Path: leaf.Path, Text: leafText(leaf.Path, leaf.Have)}, + uidiff.Row{Kind: uidiff.Add, Path: leaf.Path, Text: leafText(leaf.Path, leaf.Want)}, + ) + } + } + + return rows +} + +// leafText renders one leaf the way the plan's detail lines spell a value, +// reusing format so a composite still reads as a summary rather than a dump. +func leafText(path string, v any) string { + return path + ": " + format(v) +} + +// unmanagedNote is the diff's whole account of the live object's unmanaged +// fields, one counted line rather than a marking per field. Each such field +// is live state the file declines to manage, so none of them is a removal, +// and interleaving them with the rows would put them next to edits they have +// nothing to do with. +func unmanagedNote(paths []string) string { + if len(paths) == 0 { + return "" + } + + return tui.HintStyle.Render(fmt.Sprintf("%d %s not managed by this file", + len(paths), plural(len(paths), "field", "fields"))) +} + // settledVerdict is what an empty plan means, which is not always that there // is nothing to do. // @@ -329,6 +549,19 @@ func lockLines(plan Plan) []string { // A version that keeps the running image says so: --dry-run is the whole of the // review a deploy gets. func artifactLines(plan Plan) []string { + head := artifactEntry(plan) + if head == nil { + return nil + } + + return append(head, details(plan.Artifact)...) +} + +// artifactEntry is the artifact line without its detail list, which --diff +// replaces with the full rows underneath. It stays in diff mode because the +// one fact it carries, that a new version will be minted, is not visible in +// any field value. +func artifactEntry(plan Plan) []string { if !plan.RollsArtifact() { return nil } @@ -343,7 +576,7 @@ func artifactLines(plan Plan) []string { reason += "; keeps the running image, so no rebuild" } - return append([]string{entry("+", "artifact", reason)}, details(plan.Artifact)...) + return []string{entry("+", "artifact", reason)} } // runtimeLines describes a sizing change, which needs no new version. A @@ -473,6 +706,44 @@ type PlanJSON struct { Code CodeJSON `json:"code"` Artifact []string `json:"artifact"` Runtime []string `json:"runtime"` + + // Diff carries the structured form of what --diff renders, and is + // present only when --diff was asked for: a caller that never passed the + // flag reads the same document it always did, and one that did gets the + // changes as data rather than scraping them out of the human block. + Diff *DiffJSON `json:"diff,omitempty"` +} + +// DiffJSON is the plan's diff section. Both of its lists are the plan's own +// accounting, carried so a machine-readable envelope says everything the +// human diff does about what moves and what is left alone. +type DiffJSON struct { + // Changes is one entry per changing leaf of the manifest, in the order + // the diff body prints them. The unchanged leaves the diff draws as + // context are not here: a list of everything the file already agrees + // with would bury the few entries a consumer acts on. + Changes []ChangeJSON `json:"changes"` + + // Unmanaged holds the paths of what the live object carries that the + // file never names, so a caller can see what this deploy leaves alone + // without parsing the count out of the human summary. + Unmanaged []string `json:"unmanaged"` +} + +// ChangeJSON is one changing leaf, the structured twin of the `- old`/`+ new` +// pair the diff prints for it. Have is null and Absent true for a leaf the +// live object does not carry, which is an addition and not a swap. +type ChangeJSON struct { + Path string `json:"path"` + Have any `json:"have"` + Want any `json:"want"` + Absent bool `json:"absent"` + + // Redacted marks a path whose values were withheld, so an entry with + // null sides reads as refused rather than empty. The omitempty keeps the + // marker off the entries that carried their values in plain, which is + // most of them. + Redacted bool `json:"redacted,omitempty"` } // CodeJSON is the working tree's part of the answer. @@ -529,3 +800,208 @@ func describeAll(changes []Change) []string { return out } + +// JSONWithDiff is JSON plus the diff section --diff renders. Both come from +// the one plan, so the envelope and the human block cannot disagree about +// what changes; the plain JSON() keeps the default path's exact shape, which +// is what a caller that never passed the flag is owed. +func (p Plan) JSONWithDiff() PlanJSON { + out := p.JSON() + out.Diff = p.diffJSON() + + return out +} + +// diffJSON projects the diff rows into their machine-readable shape, the +// artifact side first with the synthetic id and type entries where Build +// appended them, then the runtime side: the order the diff body draws, so +// the two renderings of one plan list their changes as one sequence a +// consumer can zip. +func (p Plan) diffJSON() *DiffJSON { + return &DiffJSON{ + Changes: changesJSON(p.DiffArtifact, p.DiffRuntime), + Unmanaged: unmanagedJSON(p.Unmanaged), + } +} + +// changesJSON walks the two row halves keeping only the changing leaves. The +// agreeing ones are context on the way to a reader and noise in a list, and +// the walk they come from already put the changing ones in the body's order. +func changesJSON(artifact, runtime []DiffRow) []ChangeJSON { + leaves := make([]DiffRow, 0, len(artifact)+len(runtime)) + + leaves = append(leaves, artifact...) + leaves = append(leaves, runtime...) + + out := make([]ChangeJSON, 0, len(leaves)) + + for _, leaf := range leaves { + if !leaf.Changed { + continue + } + + out = append(out, changeJSON(leaf)) + } + + return out +} + +// changeJSON serialises one leaf, redacting before it marshals. The values +// of an environment variable are withheld here exactly as the human diff +// withholds them, because a machine-readable plan is the one that ends up in +// CI artifacts; the entry still says which path moved, so the change itself +// is not silently lost to the refusal. +// +// Path-based redaction alone is not enough, twice over. A NEW name-keyed list +// element arrives as one whole-element row (a new container is one row for +// the container, not one per field), so the row's own path names the +// container and never trips redacted() while its value carries an +// environmentVars block complete with secrets. And when the live side +// carries no such list at all, there is nothing to match variables against +// by name, so the file's whole list arrives as one row whose path ENDS at +// the list -- a row that is itself the block the redaction refuses to print. +// Of the two ways to close either leak, dropping the whole entry would hide +// what the element carries from a consumer that has to review the plan, so +// the values are scrubbed instead: variable names survive, values do not, +// and the scrub copies rather than mutates because the same rows feed the +// human diff after this. +func changeJSON(leaf DiffRow) ChangeJSON { + out := ChangeJSON{ + Path: leaf.Path, + Have: leaf.Have, + Want: leaf.Want, + Absent: leaf.Absent, + } + + if redacted(leaf.Path) { + out.Have = nil + out.Want = nil + out.Redacted = true + + return out + } + + if endsAtEnvVars(leaf.Path) { + // The row IS the environmentVars list, so the scrub is the list + // scrub itself; routing the value through the generic walker would + // copy every element verbatim, because the elements carry no + // environmentVars key of their own to catch. + out.Want = scrubEnvList(leaf.Want) + out.Have = scrubEnvList(leaf.Have) + out.Redacted = true + } else if carriesEnvVars(leaf.Want) || carriesEnvVars(leaf.Have) { + out.Want = scrubEnvVars(leaf.Want) + out.Have = scrubEnvVars(leaf.Have) + out.Redacted = true + } + + return out +} + +// endsAtEnvVars reports whether a leaf's path ends at the environmentVars +// list itself, the shape the walker emits when the live side has no list to +// walk element-by-element. redacted() misses it because that hook matches +// the per-variable spelling (".environmentVars["), while this row's path +// stops at the list, so the constructor names the shape itself and scrubs +// the values exactly as it scrubs them below a whole-element row. +func endsAtEnvVars(path string) bool { + return strings.HasSuffix(path, "."+envVarsKey) +} + +// carriesEnvVars reports whether a value holds an environmentVars block +// anywhere below it, which is what turns a whole-element row into a leak +// however innocent its own path reads. +func carriesEnvVars(v any) bool { + switch typed := v.(type) { + case map[string]any: + if _, ok := typed[envVarsKey]; ok { + return true + } + + for _, child := range typed { + if carriesEnvVars(child) { + return true + } + } + case []any: + for _, child := range typed { + if carriesEnvVars(child) { + return true + } + } + } + + return false +} + +// scrubEnvVars deep-copies a composite value with every environment variable +// reduced to its name. Everything else keeps its structure, so the entry +// still says what the element carries and a consumer can tell a container +// apart from its variables; only the refuse-to-print part is dropped. +func scrubEnvVars(v any) any { + switch typed := v.(type) { + case map[string]any: + out := make(map[string]any, len(typed)) + + for key, child := range typed { + if key == envVarsKey { + out[key] = scrubEnvList(child) + + continue + } + + out[key] = scrubEnvVars(child) + } + + return out + case []any: + out := make([]any, len(typed)) + + for i, child := range typed { + out[i] = scrubEnvVars(child) + } + + return out + default: + return v + } +} + +// scrubEnvList copies one environmentVars block, keeping the names and +// nothing else. A name is the part a reader acts on; every other key of an +// element is a value or the address of one, whether a plaintext literal or a +// dr-credential ref, so none of it survives the copy. An element without a +// usable name has nothing safe to say and is dropped rather than passed +// through. +func scrubEnvList(v any) []any { + list, ok := v.([]any) + if !ok { + return nil + } + + out := make([]any, 0, len(list)) + + for _, item := range list { + element, ok := item.(map[string]any) + if !ok { + continue + } + + name, ok := element["name"].(string) + if !ok || name == "" { + continue + } + + out = append(out, map[string]any{"name": name}) + } + + return out +} + +// unmanagedJSON copies the unmanaged paths, always into a non-nil slice so +// the JSON carries [] rather than null for "nothing unmanaged". +func unmanagedJSON(paths []string) []string { + out := make([]string, 0, len(paths)) + + return append(out, paths...) +} diff --git a/internal/workload/up/render_diff_test.go b/internal/workload/up/render_diff_test.go new file mode 100644 index 000000000..4be20d206 --- /dev/null +++ b/internal/workload/up/render_diff_test.go @@ -0,0 +1,649 @@ +// Copyright 2026 DataRobot, Inc. and its affiliates. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package up + +import ( + "fmt" + "strings" + "testing" + + "github.com/datarobot/cli/internal/workload/manifest" + "github.com/datarobot/cli/internal/workload/sync" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// renderDiff is the diff-mode twin of render: it runs one plan through +// RenderDiff and hands back the bytes, so the tests below can assert on +// output the way render_test.go does. +func renderDiff(t *testing.T, s Summary, plan Plan) string { + t.Helper() + + var b strings.Builder + + require.NoError(t, RenderDiff(&b, s, plan)) + + return b.String() +} + +// TestRenderDiff_SizingChangeIsAUnifiedDiff is the shape the flag exists for: +// a moved value states both sides of itself, with the file's value leaving +// and the manifest's value arriving, where the default plan would print one +// "have -> want" line. +func TestRenderDiff_SizingChangeIsAUnifiedDiff(t *testing.T) { + plan := Plan{ + State: StateRunning, + Runtime: []Change{ + {Path: "containerGroups[default].replicaCount", Have: 1.0, Want: 3.0}, + }, + DiffRuntime: []DiffRow{ + {Path: "containerGroups[default].replicaCount", Want: 3.0, Have: 1.0, Changed: true}, + }, + } + + out := renderDiff(t, appSummary, plan) + + assert.Contains(t, out, "- containerGroups[default].replicaCount: 1") + assert.Contains(t, out, "+ containerGroups[default].replicaCount: 3") +} + +// TestRenderDiff_CollapsesUnchangedRunsBeyondTheWindow pins the git +// convention the renderer inherits from uidiff: agreeing leaves stay visible +// within three lines of a change and collapse into a counted summary beyond +// it, so a big spec still renders shorter than itself without ever hiding a +// change. +func TestRenderDiff_CollapsesUnchangedRunsBeyondTheWindow(t *testing.T) { + rows := make([]DiffRow, 0, 13) + + for i := 1; i <= 6; i++ { + rows = append(rows, DiffRow{Path: fmt.Sprintf("spec.leaf%02d", i), Want: float64(i), Have: float64(i)}) + } + + rows = append(rows, DiffRow{Path: "spec.port", Want: 9090.0, Have: 8080.0, Changed: true}) + + for i := 7; i <= 12; i++ { + rows = append(rows, DiffRow{Path: fmt.Sprintf("spec.leaf%02d", i), Want: float64(i), Have: float64(i)}) + } + + // The Change list rides along because Build always fills it beside the + // rows, and plan.Empty() reads it rather than the rows: the diff's + // verdict has to mean what the run's exit code means. + out := renderDiff(t, appSummary, Plan{ + State: StateRunning, + Artifact: []Change{{Path: "spec.port", Have: 8080.0, Want: 9090.0}}, + DiffArtifact: rows, + }) + + // Three unchanged leaves on the far side of each window edge hide; the + // summary counts them out loud rather than leaving the reader to guess. + assert.Equal(t, 2, strings.Count(out, "... 3 identical lines")) + assert.NotContains(t, out, "spec.leaf01", "beyond the window, context collapses") + assert.NotContains(t, out, "spec.leaf03") + assert.Contains(t, out, "spec.leaf04", "the row at the window edge stays") + assert.Contains(t, out, "spec.leaf09", "the last row inside the far window stays") + assert.NotContains(t, out, "spec.leaf10") +} + +// TestRenderDiff_NeverTruncates is the other half of what --diff is bought +// for: the default plan caps its detail list at detailLimit and counts the +// rest, because a plan is a summary. A diff is the detail, so every changed +// leaf appears no matter how many there are. +func TestRenderDiff_NeverTruncates(t *testing.T) { + rows := make([]DiffRow, 0, 10) + + changes := make([]Change, 0, 10) + + for _, name := range []string{"a", "b", "c", "d", "e", "f", "g", "h", "i", "j"} { + rows = append(rows, DiffRow{Path: "spec." + name, Want: 2.0, Have: 1.0, Changed: true}) + + changes = append(changes, Change{Path: "spec." + name, Have: 1.0, Want: 2.0}) + } + + out := renderDiff(t, appSummary, Plan{State: StateRunning, Artifact: changes, DiffArtifact: rows}) + + assert.Contains(t, out, "- spec.j: 1") + assert.Contains(t, out, "+ spec.j: 2") + assert.NotContains(t, out, "more", "nothing is dropped, so nothing is counted out loud") +} + +// TestRenderDiff_NeverPrintsEnvironmentVariableValues is the hard rule, run +// across all three row states at once: a literal that moved, a literal that +// agrees, and a credential reference that rotated. The values behind any +// `.environmentVars[` path stay out of the output entirely -- additions, +// removals and context alike -- while the variable names stay visible, +// because the names are what tell the reader what changed. +func TestRenderDiff_NeverPrintsEnvironmentVariableValues(t *testing.T) { + const ( + literalOld = "sk-live-abc123" + literalNew = "sk-live-xyz789" + literalSame = "hunter2" + credOld = "66f1a2b3c4d5e6f7a8b9c0d1" + credNew = "aaaaaaaaaaaaaaaaaaaaaaaa" + ) + + plan := Plan{ + State: StateRunning, + Artifact: []Change{ + { + Path: "containerGroups[default].containers[primary].environmentVars[OPENAI_API_KEY].value", + Have: literalOld, + Want: literalNew, + }, + { + Path: "containerGroups[default].containers[primary].environmentVars[HUGGING_FACE_HUB_TOKEN].drCredentialId", + Have: credOld, + Want: credNew, + }, + }, + DiffArtifact: []DiffRow{ + { + Path: "containerGroups[default].containers[primary].environmentVars[OPENAI_API_KEY].value", + Have: literalOld, + Want: literalNew, + Changed: true, + }, + { + Path: "containerGroups[default].containers[primary].environmentVars[PLAINTEXT].value", + Have: literalSame, + Want: literalSame, + }, + { + Path: "containerGroups[default].containers[primary].environmentVars[HUGGING_FACE_HUB_TOKEN].drCredentialId", + Have: credOld, + Want: credNew, + Changed: true, + }, + { + Path: "containerGroups[default].containers[primary].environmentVars[FOO].name", + Have: "FOO", + Want: "FOO", + }, + }, + } + + out := renderDiff(t, appSummary, plan) + + for _, secret := range []string{literalOld, literalNew, literalSame, credOld, credNew} { + assert.NotContains(t, out, secret) + } + + assert.Contains(t, out, "environmentVars[OPENAI_API_KEY].value: changed") + assert.Contains(t, out, "environmentVars[OPENAI_API_KEY].value: set") + assert.Contains(t, out, "environmentVars[PLAINTEXT].value: (redacted)") + assert.Contains(t, out, "environmentVars[HUGGING_FACE_HUB_TOKEN].drCredentialId: changed") + assert.Contains(t, out, "environmentVars[FOO].name: (redacted)", + "a name leaf inside the block sits on a redacted path, so its value is withheld with the rest") +} + +// TestRenderDiff_NameLeavesRenderAsContext records the render-time choice +// about the name leaves DiffRows emits mechanically. They stay: inside a +// container's run of rows, the name leaf is the only line that says which +// element the surrounding lines belong to, and dropping it would leave a +// two-container diff ambiguous. They are context, never changes. +func TestRenderDiff_NameLeavesRenderAsContext(t *testing.T) { + plan := Plan{ + State: StateRunning, + Artifact: []Change{ + {Path: "containerGroups[default].containers[primary].port", Have: 8080.0, Want: 9090.0}, + }, + DiffArtifact: []DiffRow{ + {Path: "containerGroups[default].containers[primary].name", Have: "primary", Want: "primary"}, + {Path: "containerGroups[default].containers[primary].port", Have: 8080.0, Want: 9090.0, Changed: true}, + }, + } + + out := renderDiff(t, appSummary, plan) + + assert.Contains(t, out, "containerGroups[default].containers[primary].name: primary") + assert.NotContains(t, out, "- containerGroups[default].containers[primary].name") +} + +// TestRenderDiff_MultiBlockChangesRenderInOneDiff: a plan that moves the +// artifact spec and the runtime sizing renders both halves in one diff, +// because they are one deploy and the reader reviews them together. +func TestRenderDiff_MultiBlockChangesRenderInOneDiff(t *testing.T) { + drifted := `{ + "name": "my-app", + "artifact": {"name": "my-app-artifact", "spec": { + "type": "service", + "containerGroups": [{"name": "default", "containers": [ + {"name": "primary", "primary": true, "port": 9090, + "readinessProbe": {"path": "/health", "port": 8080}} + ]}] + }}, + "runtime": {"containerGroups": [ + {"name": "default", "replicaCount": 3, + "containers": [{"name": "primary", "resourceAllocation": {"cpu": 0.5, "memory": "512MB"}}]} + ]} + }` + + plan, err := Build( + loadedFrom(drifted), + liveFrom(t, StateRunning, planLiveSpec, planLiveRuntime), + builtCode(0), + Options{}, + ) + require.NoError(t, err) + + out := renderDiff(t, appSummary, plan) + + assert.Contains(t, out, "- containerGroups[default].containers[primary].port: 8080") + assert.Contains(t, out, "+ containerGroups[default].containers[primary].port: 9090") + assert.Contains(t, out, "- containerGroups[default].replicaCount: 1") + assert.Contains(t, out, "+ containerGroups[default].replicaCount: 3") +} + +// TestRenderDiff_AlreadyUpToDateIsTheDefaultVerdict: an empty plan with +// nothing unmanaged has nothing for a diff to draw, so --diff prints exactly +// what the default mode prints. The run's outcome, and its exit code, are +// the same run's. +func TestRenderDiff_AlreadyUpToDateIsTheDefaultVerdict(t *testing.T) { + plan := Plan{State: StateRunning, Code: builtCode(0)} + + out := renderDiff(t, appSummary, plan) + + assert.Equal(t, render(t, appSummary, plan), out) + assert.Contains(t, out, "โœ“ Already up to date") +} + +// TestRenderDiff_StateLinesRenderAsActionLines: stopped, errored and +// terminated are reasons the run will act, not field changes, so they keep +// the entry form and position the default plan gives them. Inside a diff +// they must not read as edits to fields nobody wrote. +func TestRenderDiff_StateLinesRenderAsActionLines(t *testing.T) { + tests := []struct { + name string + plan Plan + want string + }{ + {"stopped", Plan{State: StateStopped, Code: builtCode(0)}, "~ workload"}, + {"errored", Plan{State: StateErrored}, "! workload"}, + {"terminated", Plan{State: StateTerminated}, "! workload"}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + out := renderDiff(t, appSummary, tc.plan) + + assert.Contains(t, out, " "+tc.want, "an entry line, indented like the default plan's") + assert.NotContains(t, out, "\n- "+tc.want) + assert.NotContains(t, out, "\n+ "+tc.want) + }) + } +} + +// The state line survives alongside the field rows, and the rows still +// render as rows: the start and the sizing move in the same diff because +// they happen in the same deploy. +func TestRenderDiff_StoppedWorkloadRendersStartAndRowsTogether(t *testing.T) { + plan := Plan{ + State: StateStopped, + DiffRuntime: []DiffRow{ + {Path: "containerGroups[default].replicaCount", Want: 3.0, Have: 1.0, Changed: true}, + }, + } + + out := renderDiff(t, appSummary, plan) + + assert.Contains(t, out, "started, having been stopped") + assert.Contains(t, out, "- containerGroups[default].replicaCount: 1") + assert.Contains(t, out, "+ containerGroups[default].replicaCount: 3") +} + +// TestRenderDiff_LockLineRendersAsAnActionLine: locking is a consequence of +// the deploy, not an edit to a field, so the ~ entry keeps the form and +// position the default plan gives it and never becomes a hunk. +func TestRenderDiff_LockLineRendersAsAnActionLine(t *testing.T) { + plan := Plan{ + State: StateRunning, + Locked: true, + Code: builtCode(1), + DiffArtifact: []DiffRow{ + {Path: "port", Want: 9090.0, Have: 8080.0, Changed: true}, + }, + } + + out := renderDiff(t, appSummary, plan) + + assert.Contains(t, out, "~ lock") + assert.Contains(t, out, "locked to match") + assert.NotContains(t, out, "- lock") + assert.NotContains(t, out, "+ lock") +} + +// TestRenderDiff_UnmanagedNeverBecomesARemoval is the three-state rule's +// sharp edge. What the live object carries that the file never names renders +// once, as a counted summary; it must never appear as a `-` line, because +// the file leaving a field out is the file declining to manage it, not +// asking for it to be deleted. +func TestRenderDiff_UnmanagedNeverBecomesARemoval(t *testing.T) { + plan := Plan{ + State: StateRunning, + Runtime: []Change{ + {Path: "containerGroups[default].replicaCount", Have: 1.0, Want: 3.0}, + }, + DiffRuntime: []DiffRow{ + {Path: "containerGroups[default].replicaCount", Want: 3.0, Have: 1.0, Changed: true}, + }, + Unmanaged: []string{ + "containerGroups[default].containers[metrics]", + "containerGroups[default].containers[primary].environmentVars[FOO]", + }, + } + + out := renderDiff(t, appSummary, plan) + + assert.Contains(t, out, "2 fields not managed by this file") + assert.NotContains(t, out, "- containerGroups[default].containers[metrics]") + assert.NotContains(t, out, "- containerGroups[default].containers[primary].environmentVars[FOO]") + assert.Equal(t, 1, strings.Count(out, "not managed by this file"), + "the summary is stated once, never once per field") +} + +// TestRenderDiff_UnmanagedSummarySaysFieldOnce: the count goes through the +// same plural helper as the rest of the plan, so a lone extra does not read +// like a census. +func TestRenderDiff_UnmanagedSummarySaysFieldOnce(t *testing.T) { + plan := Plan{ + State: StateRunning, + Code: builtCode(0), + Unmanaged: []string{"containerGroups[default].containers[metrics]"}, + } + + out := renderDiff(t, appSummary, plan) + + assert.Contains(t, out, "1 field not managed by this file") +} + +// TestRenderDiff_EmptyPlanWithUnmanagedSurfacesThem is where --diff +// deliberately diverges from the default mode: Render short-circuits an +// empty plan and never mentions unmanaged fields, while the diff still says +// what the live object carries that the file never names. The verdict, and +// the exit code behind it, are unchanged. +func TestRenderDiff_EmptyPlanWithUnmanagedSurfacesThem(t *testing.T) { + plan := Plan{ + State: StateRunning, + Code: builtCode(0), + Unmanaged: []string{ + "containerGroups[default].containers[metrics]", + "containerGroups[default].containers[primary].environmentVars[FOO]", + }, + } + + out := renderDiff(t, appSummary, plan) + + assert.Contains(t, out, "โœ“ Already up to date") + assert.Contains(t, out, "2 fields not managed by this file") + assert.NotContains(t, out, "\n- ") + assert.NotContains(t, out, "\n+ ", "no managed field changed, so no change lines exist") +} + +// TestRenderDiff_FirstDeployIsAllAdditions: with no live side to compare +// against, the diff's subject is the compiled manifest itself. A header says +// so, the create summary line the default plan prints still leads, and every +// leaf of the file renders as an addition -- the diff must not be less +// informative than the plan it replaces. +func TestRenderDiff_FirstDeployIsAllAdditions(t *testing.T) { + plan, err := Build( + loadedFrom(planPayload), + Live{State: StateUnbound}, + CodeChange{Applies: true, FirstDeploy: true}, + Options{}, + ) + require.NoError(t, err) + + out := renderDiff(t, Summary{Name: "my-app"}, plan) + + assert.Contains(t, out, "my-app, first deploy") + assert.Contains(t, out, "+ workload my-app will be created, with its first artifact") + assert.Contains(t, out, "~ code all project files, uploaded for the first time") + assert.Contains(t, out, "+ containerGroups[default].containers[primary].port: 8080") + assert.Contains(t, out, "+ containerGroups[default].replicaCount: 1") + assert.NotContains(t, out, "\n- ", "there is nothing live to remove anything from") +} + +// A create that replaces a dead binding is not a first deploy, and the +// header must not claim it is: the create line's dead-binding warning is the +// whole point of that plan, exactly as in the default mode. +func TestRenderDiff_RecreateDoesNotClaimFirstDeploy(t *testing.T) { + plan, err := Build( + loadedFrom(planPayload), + Live{Live: manifest.Live{WorkloadID: "68b0c1d2e3f4a5b6c7d8e9f0"}, State: StateMissing}, + builtCode(0), + Options{}, + ) + require.NoError(t, err) + + out := renderDiff(t, Summary{Name: "my-app"}, plan) + + assert.Contains(t, out, "+ workload my-app will be created: .datarobot.yaml is bound to 68b0c1d2, "+ + "which no longer exists") + assert.NotContains(t, out, "first deploy") +} + +// TestRenderDiff_ArtifactEntryAnnouncesTheRoll: the one fact no field value +// carries is that a new version will be minted and rolled. The default +// plan's artifact entry keeps its place above the diff; its capped detail +// list is what the rows below it replace. +func TestRenderDiff_ArtifactEntryAnnouncesTheRoll(t *testing.T) { + plan := Plan{ + State: StateRunning, + Code: builtCode(0), + DiffArtifact: []DiffRow{ + {Path: "containerGroups[default].containers[primary].port", Want: 9090.0, Have: 8080.0, Changed: true}, + }, + } + + plan.Artifact = []Change{{Path: "containerGroups[default].containers[primary].port", Have: 8080.0, Want: 9090.0}} + + out := renderDiff(t, appSummary, plan) + + assert.Contains(t, out, "+ artifact new version, 1 spec change") + assert.NotContains(t, out, " containerGroups[default].containers[primary].port", + "the capped detail list is replaced by the diff, not repeated under it") +} + +// driftedSyncPlan is the dry-run plan the sync engine hands back for a tree +// with two changed files and one deleted one, wired through the codeChangeFn +// seam exactly as defaultCodeChange carries the real one. +func driftedSyncPlan() *sync.SyncPlan { + return &sync.SyncPlan{ + Uploads: []sync.FileAction{ + {Path: "main.py", Classification: sync.ClsLocalModified, Action: sync.ActUploadModify, LocalSize: 1234}, + {Path: "new.go", Classification: sync.ClsLocalAdded, Action: sync.ActUploadAdd, LocalSize: 300}, + }, + Deletes: []sync.FileAction{ + {Path: "old.txt", Action: sync.ActUploadDelete, LocalSize: 15}, + }, + } +} + +// TestRenderDiff_CodeDriftPrintsTheSyncFileList: in diff mode the code block +// is the file list the sync would upload, drawn by the same renderer +// `dr artifact code sync --dry-run` uses and fed the same dry-run plan. +// Naming the files is the whole point of a diff; a count would leave the +// reader guessing which ones move. +func TestRenderDiff_CodeDriftPrintsTheSyncFileList(t *testing.T) { + plan := Plan{ + State: StateRunning, + Locked: true, + Code: CodeChange{ + Applies: true, + Files: 3, + SyncPlan: driftedSyncPlan(), + }, + DiffArtifact: []DiffRow{ + {Path: "port", Want: 9090.0, Have: 8080.0, Changed: true}, + }, + } + + out := renderDiff(t, appSummary, plan) + + assert.Contains(t, out, "Sync plan:") + assert.Contains(t, out, "โ†‘ UPLOAD (2):") + assert.Contains(t, out, "main.py 1.2 KiB") + assert.Contains(t, out, "new.go 300 B", "rows align inside a group, the way the sync plan prints them") + assert.Contains(t, out, "โœ• DELETE (1):") + assert.Contains(t, out, "old.txt 15 B") + + assert.Contains(t, out, "~ lock", "the plan below the file list pins the block order") + + assert.NotContains(t, out, "3 files changed since the last deploy", + "the count is the default mode's summary; the diff names the files instead") + assert.NotContains(t, out, "~ code") + + assert.Less(t, strings.Index(out, "Sync plan:"), strings.Index(out, "~ lock"), + "the file list keeps the code block's position, ahead of the lock line") +} + +// TestRenderDiff_CodeCountWithoutAPlanKeepsTheSummaryLine: the file list +// needs the sync plan the seam carries, and a CodeChange built without one -- +// as test harnesses build them, since no production path measures files +// without also measuring the list -- falls back to the count the default +// mode prints rather than going silent about real drift. +func TestRenderDiff_CodeCountWithoutAPlanKeepsTheSummaryLine(t *testing.T) { + plan := Plan{ + State: StateRunning, + Code: builtCode(14), + DiffArtifact: []DiffRow{ + {Path: "port", Want: 9090.0, Have: 8080.0, Changed: true}, + }, + } + + out := renderDiff(t, appSummary, plan) + + assert.Contains(t, out, "~ code 14 files changed since the last deploy") +} + +// TestRenderDiff_UnchangedCodeRendersNoCodeEntry is the regression for the +// guard the file-list feature dropped: a plan whose drift is elsewhere -- +// here the runtime sizing -- with a tree that matches the last deploy must +// not grow a code entry, because "0 files changed since the last deploy" +// names a sync the run is never going to perform. A manifest naming an +// image has no code to sync at all, and gets the same silence. +func TestRenderDiff_UnchangedCodeRendersNoCodeEntry(t *testing.T) { + sizingOnly := Plan{ + State: StateRunning, + Code: builtCode(0), + Runtime: []Change{ + {Path: "containerGroups[default].replicaCount", Have: 1.0, Want: 3.0}, + }, + DiffRuntime: []DiffRow{ + {Path: "containerGroups[default].replicaCount", Want: 3.0, Have: 1.0, Changed: true}, + }, + } + + publishedImage := Plan{ + State: StateRunning, + Code: CodeChange{Applies: false, Files: 12}, + Runtime: []Change{ + {Path: "containerGroups[default].replicaCount", Have: 1.0, Want: 3.0}, + }, + DiffRuntime: []DiffRow{ + {Path: "containerGroups[default].replicaCount", Want: 3.0, Have: 1.0, Changed: true}, + }, + } + + for name, plan := range map[string]Plan{"unchanged tree": sizingOnly, "published image": publishedImage} { + t.Run(name, func(t *testing.T) { + out := renderDiff(t, appSummary, plan) + + assert.NotContains(t, out, "~ code") + assert.NotContains(t, out, "files changed since the last deploy") + assert.NotContains(t, out, "Sync plan:") + }) + } +} + +// TestRenderDiff_DownloadOnlySyncPlanRendersNoFileList pins the one +// predicate the code block is gated on. The deploy pushes the plan's Uploads +// and Deletes and never pulls the remote side, so a dry-run plan holding +// only downloads and conflicts -- which IsEmpty() counts and Files does not +// -- must not render a file list for a sync the run will not perform. +func TestRenderDiff_DownloadOnlySyncPlanRendersNoFileList(t *testing.T) { + plan := Plan{ + State: StateRunning, + Code: CodeChange{ + Applies: true, + SyncPlan: &sync.SyncPlan{ + Downloads: []sync.FileAction{ + { + Path: "remote-only.txt", + Classification: sync.ClsRemoteModified, + Action: sync.ActDownloadModify, + RemoteSize: 99, + }, + }, + }, + }, + Runtime: []Change{ + {Path: "containerGroups[default].replicaCount", Have: 1.0, Want: 3.0}, + }, + DiffRuntime: []DiffRow{ + {Path: "containerGroups[default].replicaCount", Want: 3.0, Have: 1.0, Changed: true}, + }, + } + + out := renderDiff(t, appSummary, plan) + + assert.NotContains(t, out, "Sync plan:") + assert.NotContains(t, out, "remote-only.txt") + assert.NotContains(t, out, "~ code") +} + +// TestRender_DefaultModeBaselineIsUnchanged pins the default plan's exact +// bytes against a baseline captured before --diff existed. The renderer grew +// fields that carry the diff rows, and this is the proof that Render neither +// reads them nor moved: same header, same capped detail list with its +// "and 1 more", same line order, byte for byte. +func TestRender_DefaultModeBaselineIsUnchanged(t *testing.T) { + changes := make([]Change, 0, 7) + + for _, name := range []string{"a", "b", "c", "d", "e", "f", "g"} { + changes = append(changes, Change{Path: "spec." + name, Have: 1.0, Want: 2.0}) + } + + plan := Plan{ + State: StateRunning, + Code: builtCode(14), + Locked: true, + Artifact: changes, + Runtime: []Change{{Path: "containerGroups[default].replicaCount", Have: 1.0, Want: 3.0}}, + DiffArtifact: []DiffRow{ + {Path: "spec.a", Want: 2.0, Have: 1.0, Changed: true}, + }, + DiffRuntime: []DiffRow{ + {Path: "containerGroups[default].replicaCount", Want: 3.0, Have: 1.0, Changed: true}, + }, + Unmanaged: []string{"containerGroups[default].containers[metrics]"}, + } + + assert.Equal(t, + "my-app (68b0c1d2), running\n"+ + "\n"+ + " ~ code 14 files changed since the last deploy\n"+ + " ~ lock the running version is locked, so a new one is created and locked to match. "+ + "Locking is permanent\n"+ + " + artifact new version, 7 spec changes\n"+ + " spec.a: 1 -> 2\n"+ + " spec.b: 1 -> 2\n"+ + " spec.c: 1 -> 2\n"+ + " spec.d: 1 -> 2\n"+ + " spec.e: 1 -> 2\n"+ + " spec.f: 1 -> 2\n"+ + " and 1 more\n"+ + " ~ runtime containerGroups[default].replicaCount: 1 -> 3\n", + render(t, appSummary, plan)) +} diff --git a/internal/workload/up/render_test.go b/internal/workload/up/render_test.go index 1c8839158..613c1f0ae 100644 --- a/internal/workload/up/render_test.go +++ b/internal/workload/up/render_test.go @@ -19,6 +19,7 @@ import ( "strings" "testing" + "github.com/datarobot/cli/internal/workload/manifest" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -530,3 +531,592 @@ func TestRender_LinkedProjectOnAPublishedImageStillGetsANewArtifact(t *testing.T assert.Contains(t, out, "with its first artifact") assert.NotContains(t, out, "linked to") } + +// diffDriftPayload is the fixture the JSON diff tests read: drift in both +// halves (two spec leaves, one whole block the live object lacks, two sizing +// leaves) with unchanged and unmanaged fields around them, which is the least +// a section claiming to mirror the diff has to carry. +const diffDriftPayload = `{ + "name": "my-app", + "artifact": {"name": "my-app-artifact", "spec": { + "type": "service", + "containerGroups": [{"name": "default", "containers": [ + {"name": "primary", "primary": true, "port": 9090, + "readinessProbe": {"path": "/ready", "port": 8080}, + "startupProbe": {"path": "/started", "port": 8080}} + ]}] + }}, + "runtime": {"containerGroups": [ + {"name": "default", "replicaCount": 3, + "containers": [{"name": "primary", "resourceAllocation": {"cpu": 1, "memory": "512MB"}}]} + ]} +}` + +// driftPlan builds that fixture the way a real run does, through Build, so +// the envelope and the diff read the same walk rather than two constructions. +func driftPlan(t *testing.T) Plan { + t.Helper() + + plan, err := Build( + loadedFrom(diffDriftPayload), + liveFrom(t, StateRunning, planLiveSpec, planLiveRuntime), + builtCode(0), + Options{}, + ) + require.NoError(t, err) + + return plan +} + +// TestPlanJSON_DiffAbsentWithoutTheFlag keeps the pre-feature envelope exact +// for a caller that never asked for a diff: no diff key at all, never a null +// one, on each of the three shapes an envelope can take. +func TestPlanJSON_DiffAbsentWithoutTheFlag(t *testing.T) { + cases := []struct { + name string + plan Plan + }{ + {"nothing changed", Plan{State: StateRunning, Code: builtCode(0)}}, + {"a sizing change", Plan{ + State: StateRunning, + Runtime: []Change{{Path: "containerGroups[default].replicaCount", Have: 1.0, Want: 3.0}}, + }}, + {"a first deploy", Plan{State: StateUnbound, Creates: true}}, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + encoded, err := json.Marshal(c.plan.JSON()) + require.NoError(t, err) + + var decoded map[string]any + + require.NoError(t, json.Unmarshal(encoded, &decoded)) + assert.NotContains(t, decoded, "diff", + "the section is added only when --diff asked for it, not nulled out") + }) + } +} + +// TestPlanJSON_DiffCarriesTheChangingLeaves: with --diff the plan carries one +// entry per changing leaf, in the walk's order, with the unchanged context +// leaves left out of it and the unmanaged paths listed beside them. The +// legacy artifact/runtime arrays ride along unchanged, because --diff adds a +// section rather than reshaping the envelope a consumer already reads. +func TestPlanJSON_DiffCarriesTheChangingLeaves(t *testing.T) { + plan := driftPlan(t) + + encoded, err := json.Marshal(plan.JSONWithDiff()) + require.NoError(t, err) + + var decoded map[string]any + + require.NoError(t, json.Unmarshal(encoded, &decoded)) + + diff, ok := decoded["diff"].(map[string]any) + require.True(t, ok, "--diff was requested, so the section is an object") + + changes, ok := diff["changes"].([]any) + require.True(t, ok, "changes is an array even when it is empty") + + got := make([]string, 0, len(changes)) + + for _, c := range changes { + entry, ok := c.(map[string]any) + require.True(t, ok) + + path, _ := entry["path"].(string) + got = append(got, path) + + assert.Contains(t, entry, "have") + assert.Contains(t, entry, "want") + assert.Contains(t, entry, "absent") + assert.NotContains(t, entry, "redacted", "nothing on this fixture is an environment variable") + } + + // One entry per changing leaf of the manifest, in the same order the + // walk reports them to the default envelope: the two halves concatenated, + // unchanged-but-managed leaves nowhere among them. + want := make([]string, 0, len(plan.Artifact)+len(plan.Runtime)) + + want = append(want, paths(plan.Artifact)...) + want = append(want, paths(plan.Runtime)...) + + assert.Equal(t, want, got) + assert.Len(t, got, 5, "the fixture drifts in five leaves across the two halves") + assert.Contains(t, got, "containerGroups[default].containers[primary].startupProbe") + assert.NotContains(t, got, "containerGroups[default].containers[primary].port.expectation", + "the walk only answers for leaves the file names") + + // The absent leaf carries no live value, and says so twice: a null have + // and an absent flag, because "not there" is a different act from + // "different value". + var added map[string]any + + for _, c := range changes { + entry, _ := c.(map[string]any) + + if entry["path"] == "containerGroups[default].containers[primary].startupProbe" { + added = entry + + break + } + } + + require.NotNil(t, added) + assert.Nil(t, added["have"]) + assert.Equal(t, true, added["absent"]) + assert.NotNil(t, added["want"]) + + // The unmanaged side is its own list, the plan's deduped Extra paths: + // the sidecar the file never names, counted once however many halves + // carry it, and never mistaken for a change. + unmanaged, ok := diff["unmanaged"].([]any) + require.True(t, ok, "unmanaged is an array even when it is empty") + require.Len(t, unmanaged, 1) + assert.Equal(t, "containerGroups[default].containers[metrics]", unmanaged[0]) + + for _, path := range unmanaged { + assert.NotContains(t, got, path, "unmanaged fields are not changes") + } + + // The arrays a consumer already reads keep their exact content with and + // without the section. + plain := plan.JSON() + + assert.Equal(t, plain.Artifact, plan.JSONWithDiff().Artifact) + assert.Equal(t, plain.Runtime, plan.JSONWithDiff().Runtime) +} + +// TestPlanJSON_DiffRedactsBeforeSerialising is the JSON half of the one hard +// rule. A literal can be a secret pasted in plaintext and a credential ref +// names one; both ride in the diff rows as raw values, so the values are +// withheld here rather than trusted to a marshalling order, and the entry +// carries a marker saying the refusal happened. +// +// The sidecar container pins the whole-element half of the rule: a NEW +// name-keyed list element is one row for the element, not one per field, so +// the row's own path names the container and never trips the path-based +// redaction while its value carries an environmentVars block complete with +// secrets. Nothing a variable holds may reach the document. +func TestPlanJSON_DiffRedactsBeforeSerialising(t *testing.T) { + const ( + literal = "sk-literal-plaintext" + oldLit = "sk-old-plaintext" + rotated = "aaaa222222222222222222aa" + liveID = "66f1a2b3c4d5e6f7a8b9c0d1" + sidecarLit = "hunter2-plaintext" + sidecarCredID = "77c2b3c4d5e6f7a8b9c0d199" + ) + + payload := `{ + "name": "my-app", + "artifact": {"name": "my-app-artifact", "spec": { + "type": "service", + "containerGroups": [{"name": "default", "containers": [ + {"name": "primary", "environmentVars": [ + {"name": "OPENAI_API_KEY", "value": "` + literal + `"}, + {"name": "HUGGING_FACE_HUB_TOKEN", "source": "dr-credential", + "drCredentialId": "` + rotated + `", "key": "apiToken"} + ]}, + {"name": "sidecar", "image": "sidecar:2", "environmentVars": [ + {"name": "SIDECAR_PASSWORD", "value": "` + sidecarLit + `"}, + {"name": "SIDECAR_API_TOKEN", "source": "dr-credential", + "drCredentialId": "` + sidecarCredID + `", "key": "sidecarKey"} + ]} + ]}] + }} + }` + + live := `{ + "type": "service", + "containerGroups": [{"name": "default", "containers": [ + {"name": "primary", "environmentVars": [ + {"name": "OPENAI_API_KEY", "value": "` + oldLit + `"}, + {"name": "HUGGING_FACE_HUB_TOKEN", "source": "dr-credential", + "drCredentialId": "` + liveID + `", "key": "apiToken"} + ]} + ]}] + }` + + plan, err := Build(loadedFrom(payload), liveFrom(t, StateRunning, live, planLiveRuntime), builtCode(0), Options{}) + require.NoError(t, err) + + // The fixture has to earn its keep: two env-var changes on the existing + // container plus the whole-element row the new container emits, and all + // three are what must not leak. + require.Equal(t, []string{ + "containerGroups[default].containers[primary].environmentVars[OPENAI_API_KEY].value", + "containerGroups[default].containers[primary].environmentVars[HUGGING_FACE_HUB_TOKEN].drCredentialId", + "containerGroups[default].containers[sidecar]", + }, paths(plan.Artifact)) + + encoded, err := json.Marshal(plan.JSONWithDiff()) + require.NoError(t, err) + + document := string(encoded) + + for _, secret := range []string{literal, oldLit, rotated, liveID, sidecarLit, sidecarCredID, "dr-credential:"} { + assert.NotContains(t, document, secret) + } + + var decoded map[string]any + + require.NoError(t, json.Unmarshal(encoded, &decoded)) + + diff := decoded["diff"].(map[string]any) + changes := diff["changes"].([]any) + require.Len(t, changes, 3) + + for _, c := range changes { + entry := c.(map[string]any) + + if entry["path"] == "containerGroups[default].containers[sidecar]" { + continue + } + + assert.Contains(t, entry["path"], "environmentVars[", "the name is the whole point of the entry") + assert.Nil(t, entry["have"], "the live value never serialises") + assert.Nil(t, entry["want"], "the asked-for value never serialises") + assert.Equal(t, true, entry["redacted"]) + } + + // The new container's entry arrives as a whole-element row: the path + // names the container, so the scrub works on the subtree instead. The + // names of its variables stay, because seeing which variables an element + // sets is half the point of the entry; every value, literal or + // credential ref, is gone, and the refusal is marked. + var sidecar map[string]any + + for _, c := range changes { + entry, _ := c.(map[string]any) + + if entry["path"] == "containerGroups[default].containers[sidecar]" { + sidecar = entry + + break + } + } + + require.NotNil(t, sidecar, "the new container is one whole-element change") + assert.Equal(t, true, sidecar["redacted"]) + assert.Nil(t, sidecar["have"], "the container is an addition") + assert.Equal(t, true, sidecar["absent"]) + + want, ok := sidecar["want"].(map[string]any) + require.True(t, ok, "the container's other fields keep their structure") + + assert.Equal(t, "sidecar:2", want["image"], "nothing secret keeps the structure honest") + + vars, ok := want["environmentVars"].([]any) + require.True(t, ok, "the variable names stay") + require.Len(t, vars, 2) + + assert.Equal(t, map[string]any{"name": "SIDECAR_PASSWORD"}, vars[0]) + assert.Equal(t, map[string]any{"name": "SIDECAR_API_TOKEN"}, vars[1]) + + // The legacy arrays were redacted before this section existed, and stay + // that way: they say the change happened, never what it said. + plain := plan.JSON() + + assert.NotContains(t, strings.Join(plain.Artifact, "\n"), literal) + assert.Contains(t, strings.Join(plain.Artifact, "\n"), "changed") + assert.Equal(t, plain.Artifact, plan.JSONWithDiff().Artifact) +} + +// TestPlanJSON_DiffRedactsAWholeEnvVarsList: when the live side carries no +// environmentVars list at all, the walker has nothing to match variables +// against by name, so the file's whole list arrives as ONE row whose path +// ENDS at the list. That row is as much a carrier of variable values as any +// whole-element row, and the document must scrub it the same way: the names +// stay, every value -- literal or credential ref -- is gone, and the refusal +// is marked. (Caught against live staging: the container-level scrub only +// saw values nested BELOW a row, never a row that was the list itself.) +func TestPlanJSON_DiffRedactsAWholeEnvVarsList(t *testing.T) { + const ( + literal = "sk-plaintext-9999" + credID = "88d3c4d5e6f7a8b9c0d1e2f3" + ) + + payload := `{ + "name": "my-app", + "artifact": {"name": "my-app-artifact", "spec": { + "type": "service", + "containerGroups": [{"name": "default", "containers": [ + {"name": "primary", "environmentVars": [ + {"name": "OPENAI_API_KEY", "value": "` + literal + `"}, + {"name": "HUGGING_FACE_HUB_TOKEN", "source": "dr-credential", + "drCredentialId": "` + credID + `", "key": "apiToken"} + ]} + ]}] + }} + }` + + // The live container exists but names no variables, so there is no list + // to walk element-by-element: the file's whole environmentVars list is + // one changed leaf. + live := `{ + "type": "service", + "containerGroups": [{"name": "default", "containers": [ + {"name": "primary", "image": "app:1"} + ]}] + }` + + plan, err := Build(loadedFrom(payload), liveFrom(t, StateRunning, live, planLiveRuntime), builtCode(0), Options{}) + require.NoError(t, err) + + // The fixture has to earn its keep: the change must arrive as the + // whole-list row, not as per-variable rows. + assert.Contains(t, paths(plan.Artifact), + "containerGroups[default].containers[primary].environmentVars", + "expected the whole environmentVars list as one changed leaf") + + encoded, err := json.Marshal(plan.JSONWithDiff()) + require.NoError(t, err) + + document := string(encoded) + + for _, secret := range []string{literal, credID, "dr-credential:"} { + assert.NotContains(t, document, secret) + } + + var decoded map[string]any + + require.NoError(t, json.Unmarshal(encoded, &decoded)) + + changes := decoded["diff"].(map[string]any)["changes"].([]any) + + var entry map[string]any + + for _, c := range changes { + e, _ := c.(map[string]any) + + if e["path"] == "containerGroups[default].containers[primary].environmentVars" { + entry = e + + break + } + } + + require.NotNil(t, entry, "the whole-list change must survive the scrub") + assert.Equal(t, true, entry["redacted"]) + + want, ok := entry["want"].([]any) + require.True(t, ok, "the variable names keep their list") + + assert.Equal(t, []any{ + map[string]any{"name": "OPENAI_API_KEY"}, + map[string]any{"name": "HUGGING_FACE_HUB_TOKEN"}, + }, want, "only the names survive the scrub") +} + +// TestPlanJSON_FirstDeployDiffIsAllAdditions: with no live object every leaf +// of the compiled manifest is an addition, absent with no live value, and +// there is nothing for the unmanaged list to hold. +func TestPlanJSON_FirstDeployDiffIsAllAdditions(t *testing.T) { + plan, err := Build( + loadedFrom(planPayload), + Live{State: StateUnbound}, + CodeChange{Applies: true, FirstDeploy: true}, + Options{}, + ) + require.NoError(t, err) + + encoded, err := json.Marshal(plan.JSONWithDiff()) + require.NoError(t, err) + + var decoded map[string]any + + require.NoError(t, json.Unmarshal(encoded, &decoded)) + + assert.Equal(t, true, decoded["creates"]) + assert.Empty(t, decoded["priorWorkloadId"]) + + diff, ok := decoded["diff"].(map[string]any) + require.True(t, ok) + + changes := diff["changes"].([]any) + require.NotEmpty(t, changes, "a first deploy is made of the leaves the manifest names") + + for _, c := range changes { + entry := c.(map[string]any) + + assert.Equal(t, true, entry["absent"], "%v must be an addition", entry["path"]) + assert.Nil(t, entry["have"]) + assert.NotNil(t, entry["want"]) + } + + unmanaged := diff["unmanaged"].([]any) + assert.Empty(t, unmanaged, "there is no live object to carry unmanaged fields") + + plain := plan.JSON() + + assert.Equal(t, plain.Artifact, plan.JSONWithDiff().Artifact) + assert.Equal(t, plain.Runtime, plan.JSONWithDiff().Runtime) +} + +// TestPlanJSON_EmptyPlanDiffIsTwoEmptyLists: --diff still asked, so the +// section is present with empty arrays where a change would sit, and the +// rest of the envelope is the exact document the default path would have +// produced. +func TestPlanJSON_EmptyPlanDiffIsTwoEmptyLists(t *testing.T) { + plan := Plan{State: StateRunning, Code: builtCode(0)} + + encoded, err := json.Marshal(plan.JSONWithDiff()) + require.NoError(t, err) + + var withDiff map[string]any + + require.NoError(t, json.Unmarshal(encoded, &withDiff)) + + diff, ok := withDiff["diff"].(map[string]any) + require.True(t, ok, "the section is present because --diff asked, even for an empty plan") + + changes, ok := diff["changes"].([]any) + require.True(t, ok, "changes is [] rather than null") + assert.Empty(t, changes) + + unmanaged, ok := diff["unmanaged"].([]any) + require.True(t, ok, "unmanaged is [] rather than null") + assert.Empty(t, unmanaged) + + plainEncoded, err := json.Marshal(plan.JSON()) + require.NoError(t, err) + + var plain map[string]any + + require.NoError(t, json.Unmarshal(plainEncoded, &plain)) + + delete(withDiff, "diff") + assert.Equal(t, plain, withDiff, "aside from the section, the two envelopes are one document") +} + +// textDiffChangePaths pulls the paths of a rendered diff's change lines, in +// order. A changed leaf states both sides of itself, `- old` then `+ new`, +// and is one entry in the JSON list, so a run of same-path lines collapses to +// one. The text and the list then say the same sequence, not just the same set. +func textDiffChangePaths(t *testing.T, body string) []string { + t.Helper() + + var out []string + + for _, line := range strings.Split(body, "\n") { + if len(line) < 2 || (line[0] != '+' && line[0] != '-') { + continue + } + + path, _, _ := strings.Cut(line[2:], ": ") + + if len(out) > 0 && out[len(out)-1] == path { + continue + } + + out = append(out, path) + } + + return out +} + +// TestPlanJSON_DiffOrderMatchesTheTextBody: both renderings draw from the one +// walk the plan was built with, so the Nth entry of the list is the Nth +// change the text prints, artifact side then runtime side, synthetic +// artifact-id change included. A consumer reading both representations +// deserves a sequence it can zip, and a list that reordered itself between +// the two would make every such zip a lie. +func TestPlanJSON_DiffOrderMatchesTheTextBody(t *testing.T) { + plan := driftPlan(t) + + var body strings.Builder + + require.NoError(t, RenderDiff(&body, appSummary, plan)) + + encoded, err := json.Marshal(plan.JSONWithDiff()) + require.NoError(t, err) + + var decoded PlanJSON + + require.NoError(t, json.Unmarshal(encoded, &decoded)) + require.NotNil(t, decoded.Diff) + require.NotEmpty(t, decoded.Diff.Changes, "the fixture drifts, so both renderings have changes to line up") + + fromJSON := make([]string, 0, len(decoded.Diff.Changes)) + + for _, c := range decoded.Diff.Changes { + fromJSON = append(fromJSON, c.Path) + } + + assert.Equal(t, textDiffChangePaths(t, body.String()), fromJSON) +} + +// TestPlanJSON_DiffSyntheticChangesSurvive: the artifact id is a change no +// walk of the spec can produce, because a file bound by id describes no spec +// at all. The default envelope reports it, so the diff section must too -- +// a version roll invisible in one rendering of the plan it drives is a +// silent regression waiting for a reader. +func TestPlanJSON_DiffSyntheticChangesSurvive(t *testing.T) { + loaded := Loaded{Compiled: &manifest.Compiled{ + Payload: json.RawMessage(`{"name": "my-app", "artifactId": "68b0bbbb0000000000000002"}`), + ArtifactID: "68b0bbbb0000000000000002", + }} + + live := liveFrom(t, StateRunning, "", planLiveRuntime) + live.ArtifactID = "68a0000000000000000000a1" + + plan, err := Build(loaded, live, builtCode(0), Options{}) + require.NoError(t, err) + + require.Equal(t, []string{"artifactId"}, paths(plan.Artifact), + "the fixture has to reach Build's synthetic append for the test to mean anything") + + encoded, err := json.Marshal(plan.JSONWithDiff()) + require.NoError(t, err) + + var decoded PlanJSON + + require.NoError(t, json.Unmarshal(encoded, &decoded)) + require.NotNil(t, decoded.Diff) + + require.Len(t, decoded.Diff.Changes, 1) + + change := decoded.Diff.Changes[0] + + assert.Equal(t, "artifactId", change.Path) + assert.Equal(t, "68a0000000000000000000a1", change.Have) + assert.Equal(t, "68b0bbbb0000000000000002", change.Want) + assert.False(t, change.Absent) +} + +// The type is the same kind of blind spot: the platform reads the +// discriminator off the artifact, so the spec walk never sees it. It rides in +// the synthetic append beside the id, and lands in the list the same way. +func TestPlanJSON_DiffSyntheticTypeChangeSurvives(t *testing.T) { + loaded := Loaded{Compiled: &manifest.Compiled{ + Payload: json.RawMessage(`{ + "name": "my-app", + "artifact": {"name": "my-app-artifact", "type": "agent", "spec": {}} + }`), + }} + + live := liveFrom(t, StateRunning, "", planLiveRuntime) + live.ArtifactType = "service" + + plan, err := Build(loaded, live, builtCode(0), Options{}) + require.NoError(t, err) + + require.Equal(t, []string{"artifact.type"}, paths(plan.Artifact)) + + encoded, err := json.Marshal(plan.JSONWithDiff()) + require.NoError(t, err) + + var decoded PlanJSON + + require.NoError(t, json.Unmarshal(encoded, &decoded)) + require.NotNil(t, decoded.Diff) + require.Len(t, decoded.Diff.Changes, 1) + + change := decoded.Diff.Changes[0] + + assert.Equal(t, "artifact.type", change.Path) + assert.Equal(t, "service", change.Have) + assert.Equal(t, "agent", change.Want) +} diff --git a/internal/workload/up/run.go b/internal/workload/up/run.go index b3b4e0773..36dc57dc0 100644 --- a/internal/workload/up/run.go +++ b/internal/workload/up/run.go @@ -68,6 +68,13 @@ var ( syncProjectFn = defaultSync ) +// ErrDeclined is what a run answers when --confirm was given and the user +// said no. It is returned before the first mutating branch runs, so a caller +// can treat it as "nothing happened" rather than as a failure partway through +// one: nothing was deployed, nothing was locked, and the plan it declined is +// still the plan the next run will carry out. +var ErrDeclined = errors.New("declined: nothing was deployed") + // Options is everything a run needs from its caller. type Options struct { // Dir is where to start looking for the manifest. The search walks @@ -81,6 +88,13 @@ type Options struct { // DryRun stops after the plan. DryRun bool + // Diff renders the plan as a unified diff instead of the summary list: + // the same plan, laid out leaf by leaf with context around what moves. + // It changes how the plan is shown and nothing else -- the dry-run + // return and every apply branch read the plan exactly as they do + // without it, so a diff run and a summary run mutate the same things. + Diff bool + // Detach returns once the apply is requested, skipping the waits. Detach bool @@ -97,6 +111,16 @@ type Options struct { // caller's job because only it knows where the user's input comes from. Confirm func(question, want string) (bool, error) + // ConfirmApply is the opt-in y/N gate behind --confirm, and nil when + // nobody is to be asked. It is separate from Confirm: that one is the + // locked-production question only the exact workload name answers, and + // this one is the ordinary "apply this?" asked of every run the flag was + // given to. The deploy calls it once, after the plan is printed and + // before the first mutating branch, and a no stops the run with + // ErrDeclined. Reading the answer is the caller's job because only it + // knows where the user's input comes from. + ConfirmApply func(question string) (bool, error) + // Lock makes the artifact that ends up live immutable and permanent. // Locking is one-way, so it happens last, only after the workload is // actually serving: locking something that never came up would leave an @@ -198,12 +222,24 @@ func Run(opts Options) (Result, error) { } summary := Summary{Name: result.Name, WorkloadID: result.WorkloadID, Status: live.Status} - if err := Render(opts.Stderr, summary, plan); err != nil { + + renderPlan := rendererFor(opts.Diff) + + if err := renderPlan(opts.Stderr, summary, plan); err != nil { return result, err } noteUnusedForce(plan, opts) + return carryOut(loaded, live, plan, result, opts) +} + +// carryOut is everything a run does once the plan has been shown: stop for a +// dry run, ask the confirm gate, then act on the plan through whichever path +// applies. It is its own step because the ordering here is the contract the +// --confirm gate rests on: the review is printed before the question is asked, +// and the question is answered before the first mutation is attempted. +func carryOut(loaded Loaded, live Live, plan Plan, result Result, opts Options) (Result, error) { if opts.DryRun { // The one run whose action is the plan's: it stops here, so printing // the plan is the whole of what it did. @@ -212,6 +248,10 @@ func Run(opts Options) (Result, error) { return result, nil } + if err := confirmGate(plan, result, opts); err != nil { + return result, err + } + if plan.Empty() { return lockOnly(loaded, live, result, opts) } @@ -219,6 +259,55 @@ func Run(opts Options) (Result, error) { return apply(loaded, live, plan, result, opts) } +// confirmGate asks the opt-in y/N question --confirm installs, and only when +// answering yes would change something. The gate fires whenever the run would +// otherwise mutate: a plan carrying changes, or an empty one with a --lock +// waiting to make the serving artifact permanent. A wholly empty plan asks +// nothing and returns as it always has, and unmanaged fields never arm the +// gate: they survive every deploy untouched, so there is nothing to consent +// to. "Empty" is therefore measured by pending mutation, not by how quiet the +// plan looks. +// +// Everything that prints has printed by the time the question is asked, and +// everything that mutates is still ahead of it. That placement is the whole +// promise: a decline returns ErrDeclined from a run that has changed nothing, +// and a dry run never gets here at all -- it returns above, because a preview +// is not a mutation to consent to. +func confirmGate(plan Plan, result Result, opts Options) error { + if opts.ConfirmApply == nil { + return nil + } + + // lockOnly is the one mutation an empty plan can still carry out, and + // only when --lock was passed and the serving artifact is not already + // locked. Any other empty plan is the "Already up to date" run. + if plan.Empty() && (result.Locked || !opts.Lock) { + return nil + } + + agreed, err := opts.ConfirmApply("Apply this deploy?") + if err != nil { + return fmt.Errorf("cannot ask to apply this deploy: %w", err) + } + + if !agreed { + return ErrDeclined + } + + return nil +} + +// rendererFor picks how the plan is shown. --diff swaps the renderer and +// nothing else: the plan goes out laid out as a diff rather than summarised, +// and everything below reads it identically either way. +func rendererFor(diff bool) func(io.Writer, Summary, Plan) error { + if diff { + return RenderDiff + } + + return Render +} + // lockOnly is the whole of a --lock run that found nothing else to do. // // --lock is about the end state rather than about what this run happened to @@ -1267,8 +1356,13 @@ func defaultCodeChange(loaded Loaded, live Live) (change CodeChange, err error) } return CodeChange{ - Applies: true, - Files: len(plan.Uploads) + len(plan.Deletes), + Applies: true, + Files: len(plan.Uploads) + len(plan.Deletes), + // The plan rides along beside its count so --diff can name the files + // rather than only counting them, rendered by the sync command's own + // plan printer. It is data, not engine state, so it survives the + // Close below. + SyncPlan: plan, IgnoreNotice: notice, ImageStale: imageStale(loaded.ProjectDir, live), // The engine says so rather than this asking a second time: it fetched diff --git a/internal/workload/up/run_test.go b/internal/workload/up/run_test.go index dfa942f66..e8069117d 100644 --- a/internal/workload/up/run_test.go +++ b/internal/workload/up/run_test.go @@ -502,6 +502,152 @@ func TestRun_DryRunAppliesNothing(t *testing.T) { assert.Contains(t, stderr, "+ workload") } +// resizedManifest is the bound fixture with the sizing moved one step, which +// is the one-change plan the wiring tests below diff. The binding line rides +// on top, as it does wherever the tests deploy against the live fixtures. +func resizedManifest() string { + return "workloadId: 68b0c1d2e3f4a5b6c7d8e9f0\n" + + strings.Replace(boundLiveManifest, "memory: 22GB", "memory: 24GB", 1) +} + +// TestRun_DiffRendersTheUnifiedDiffInsteadOfTheSummary is the selector: the +// flag swaps the renderer and nothing else. With it the plan is laid out as +// a diff stating both sides of what moves; without it the summary keeps its +// own spelling, because a flag nobody passed may not change a byte. +func TestRun_DiffRendersTheUnifiedDiffInsteadOfTheSummary(t *testing.T) { + install(t, fakes{ + workloadD: func(string) (workload.Document, error) { return doc(t, liveWorkloadJSON), nil }, + artifactD: func(string) (workload.Document, error) { return doc(t, liveArtifactJSON), nil }, + }) + + sized := resizedManifest() + + _, stderr, err := runIn(t, sized, Options{NonInteractive: true, DryRun: true}) + require.NoError(t, err) + + assert.Contains(t, stderr, "resourceAllocation.memory: 22GB -> 24GB", + "the default plan states a change as have -> want") + assert.NotContains(t, stderr, "\n+ ") + assert.NotContains(t, stderr, "\n- ", "the diff is opt-in; the default plan must not grow hunks") + + _, diffStderr, err := runIn(t, sized, Options{NonInteractive: true, DryRun: true, Diff: true}) + require.NoError(t, err) + + assert.Contains(t, diffStderr, "- containerGroups[default].containers[vllm-server].resourceAllocation.memory: 22GB") + assert.Contains(t, diffStderr, "+ containerGroups[default].containers[vllm-server].resourceAllocation.memory: 24GB") + assert.NotContains(t, diffStderr, "-> ", + "a diff states each side of a change on its own line, never the summary's arrow") +} + +// TestRun_DryRunDiffMatchesTheWetDiffAndWritesNothing is the contract that +// makes --dry-run --diff the look-without-touching combination: the dry run +// prints the identical diff body the wet run prints for the same state, and +// stops where the wet run continues into progress. The mutating seams are +// wired to fail the test on the dry leg, so "prints the same body" and +// "performs zero writes" are asserted by the same run. +func TestRun_DryRunDiffMatchesTheWetDiffAndWritesNothing(t *testing.T) { + sized := resizedManifest() + + liveDocs := fakes{ + workloadD: func(string) (workload.Document, error) { return doc(t, liveWorkloadJSON), nil }, + artifactD: func(string) (workload.Document, error) { return doc(t, liveArtifactJSON), nil }, + } + + dry := liveDocs + dry.settings = func(string, json.RawMessage) (*workload.Replacement, error) { + t.Fatal("a dry run must not touch the workload") + + return nil, nil + } + dry.replace = func(string, string, json.RawMessage) (*workload.Replacement, error) { + t.Fatal("a dry run must not roll anything") + + return nil, nil + } + + install(t, dry) + + dryResult, dryStderr, err := runIn(t, sized, Options{NonInteractive: true, DryRun: true, Diff: true}) + require.NoError(t, err) + + assert.Equal(t, ActionUpdated, dryResult.Action, "the dry run's action is the plan's own") + assert.Contains(t, dryStderr, "+ containerGroups[default].containers[vllm-server].resourceAllocation.memory: 24GB") + + // The wet leg runs the same plan far enough to act: the resize is + // requested, and --detach stops there so the leg needs no wait wiring. + var resized bool + + wet := liveDocs + wet.settings = func(string, json.RawMessage) (*workload.Replacement, error) { + resized = true + + return nil, nil + } + + install(t, wet) + + _, wetStderr, err := runIn(t, sized, Options{NonInteractive: true, Diff: true, Detach: true}) + require.NoError(t, err) + assert.True(t, resized, "the wet leg has to reach the mutation for the comparison to mean anything") + + assert.True(t, strings.HasPrefix(wetStderr, dryStderr), + "the wet run must print the identical diff body before its progress;\ndry: %q\nwet: %q", dryStderr, wetStderr) +} + +// TestRun_PlanJSONDiffIsTheSameDryAndWet is that equivalence for the machine +// envelope: the diff section is computed from the pre-apply plan, so the dry +// preview and the run that follows it hand a consumer the same changes and +// the same unmanaged paths, in the same order. The action differs between +// the two; the section must not, or a script diffing the two envelopes would +// read drift into a deploy that fixed it. +func TestRun_PlanJSONDiffIsTheSameDryAndWet(t *testing.T) { + sized := resizedManifest() + + liveDocs := fakes{ + workloadD: func(string) (workload.Document, error) { return doc(t, liveWorkloadJSON), nil }, + artifactD: func(string) (workload.Document, error) { return doc(t, liveArtifactJSON), nil }, + } + + dry := liveDocs + dry.settings = func(string, json.RawMessage) (*workload.Replacement, error) { + t.Fatal("a dry run must not touch the workload") + + return nil, nil + } + dry.replace = func(string, string, json.RawMessage) (*workload.Replacement, error) { + t.Fatal("a dry run must not roll anything") + + return nil, nil + } + + install(t, dry) + + dryResult, _, err := runIn(t, sized, Options{NonInteractive: true, DryRun: true, Diff: true}) + require.NoError(t, err) + + dryDiff := dryResult.Plan.JSONWithDiff().Diff + require.NotNil(t, dryDiff) + require.NotEmpty(t, dryDiff.Changes, + "the fixture has to move something for the comparison to mean anything") + + var resized bool + + wet := liveDocs + wet.settings = func(string, json.RawMessage) (*workload.Replacement, error) { + resized = true + + return nil, nil + } + + install(t, wet) + + wetResult, _, err := runIn(t, sized, Options{NonInteractive: true, Diff: true, Detach: true}) + require.NoError(t, err) + assert.True(t, resized, "the wet leg has to reach the mutation for the comparison to mean anything") + + assert.Equal(t, dryDiff, wetResult.Plan.JSONWithDiff().Diff) +} + // TestRun_AlreadyUpToDate exits without touching anything, which is what // makes `up` cheap to run on every push. func TestRun_AlreadyUpToDate(t *testing.T) { diff --git a/smoke_test_scripts/workload/RAPTOR-19538-diff-confirm.sh b/smoke_test_scripts/workload/RAPTOR-19538-diff-confirm.sh new file mode 100755 index 000000000..fa3085002 --- /dev/null +++ b/smoke_test_scripts/workload/RAPTOR-19538-diff-confirm.sh @@ -0,0 +1,512 @@ +#!/usr/bin/env bash +# Ticket: RAPTOR-19538 +# Live staging smoke โ€” `dr workload up --diff` + `--confirm` (m3-live-smoke). +# +# Deploys a throwaway image-based whoami workload on staging (the RAPTOR-19533 +# scenario A pattern: create --spec-file โ†’ wait running โ†’ config binds and +# renders .datarobot.yaml), edits the manifest's sizing plus two environment +# variables (one credential-backed `dr-credential:/` reference and +# one literal plaintext value), then walks the six VAL-SMOKE assertions: +# +# SMOKE-001 `up --dry-run --diff` shows the sizing change as a unified +# diff (- old / + new with context) and leaks no env-var value. +# SMOKE-002 A real `up --confirm` answered "n" exits NONZERO and mutates +# nothing. Run under a PTY via expect(1) with +# DATAROBOT_CLI_NON_INTERACTIVE unset: lib.sh sets the variable +# and a piped stdin would suppress the gate, so both overrides +# are required for the prompt to install at all. +# SMOKE-005 `up --dry-run --diff --output-format json` emits one JSON +# document whose plan.diff carries the structured change list, +# with env-var values redacted, while the change is still +# pending (i.e. before SMOKE-003 applies it). +# SMOKE-003 A real `up --confirm` answered "y" applies the sizing change: +# a follow-up `up --dry-run` reports the workload up to date, +# which is the CLI's own live-vs-manifest verdict that sizing +# now matches the manifest (`workload get` does not surface +# sizing, so the dry-run plan is the observable). +# SMOKE-004 Cleanup: the EXIT-trap cleanup deletes the created workload +# and artifact, and a post-cleanup probe proves no RUN-prefixed +# resource is left on staging. +# +# The decline leg (SMOKE-002) expects a NONZERO exit, so it is wrapped in a +# `set +e` guard and asserts the captured code directly โ€” never +# wl::assert_cmd_ok, which would abort the script on the expected failure and +# strand SMOKE-003. +# +# ~15 min. Run directly (serially; it creates/deletes real staging +# resources): smoke_test_scripts/workload/RAPTOR-19538-diff-confirm.sh +# +# Transcript/evidence files land in +# tmp/smoke-19538// (override with SMOKE_TRANSCRIPT_DIR). + +# shellcheck shell=bash +set -euo pipefail +source "$(dirname "${BASH_SOURCE[0]}")/lib.sh" + +wl::init_env +wl::register_cleanup + +# The lib trap stops workloads, but a stopped draft still shows up in +# `dr workload list`, and SMOKE-004 requires no RUN-prefixed leftovers at +# all. Keep the lib registration (the registry helpers below are the same +# arrays wl::cleanup drains) and swap the EXIT trap for a stronger wrapper +# that DELETEs our workloads (the platform stops a running workload before +# removing it) and then hands off to the lib cleanup for artifacts, scratch, +# and exit-code propagation. +smoke::cleanup_resources() { + local id + + # Workloads first: an artifact backing a live workload cannot be deleted. + for id in "${WL_CREATED_WORKLOADS[@]:-}"; do + [[ -n "$id" ]] || continue + "$DR_BIN" workload delete "$id" --yes >/dev/null 2>&1 || true + echo " ๐Ÿงน deleted workload $id" + done + WL_CREATED_WORKLOADS=() + + for id in "${WL_ARTIFACT_IDS[@]:-}"; do + [[ -n "$id" ]] || continue + "$DR_BIN" artifact delete "$id" --yes >/dev/null 2>&1 || true + echo " ๐Ÿงน deleted artifact $id" + done + WL_ARTIFACT_IDS=() +} + +smoke::cleanup() { + local rc=$? + + smoke::cleanup_resources + + [[ -n "${WL_SCRATCH:-}" ]] && rm -rf "$WL_SCRATCH" 2>/dev/null || true + + return "$rc" +} +# shellcheck disable=SC2317 # reached via the EXIT trap, not a call site. +trap smoke::cleanup EXIT + +# --------------------------------------------------------------------------- +# Evidence: transcripts land outside the scratch dir, which the trap removes. +# --------------------------------------------------------------------------- +TRANSCRIPT_DIR="${SMOKE_TRANSCRIPT_DIR:-$WL_REPO_ROOT/tmp/smoke-19538/$RUN}" +mkdir -p "$TRANSCRIPT_DIR" + +smoke::record() { + # record : persist this leg's WL_OUT/WL_RC/WL_ERR capture. + local leg=$1 + printf '%s' "$WL_OUT" > "$TRANSCRIPT_DIR/$leg.out" + printf '%s' "$WL_ERR" > "$TRANSCRIPT_DIR/$leg.err" + echo "$WL_RC" > "$TRANSCRIPT_DIR/$leg.rc" +} + +# --------------------------------------------------------------------------- +# PTY driver for the interactive --confirm gate (SMOKE-002/003). +# +# The gate installs only when stdin is a terminal AND the run is interactive. +# This bash script's stdin is a pipe and wl::init_env exported +# DATAROBOT_CLI_NON_INTERACTIVE=1 โ€” either one alone suppresses the prompt BY +# DESIGN, so the driver runs the CLI under expect(1), which allocates a real +# PTY, and unsets the variable in the child environment. The answer ("n" or +# "y") is fed to the prompt through the PTY like a keystroke. +# --------------------------------------------------------------------------- +smoke::write_expect_driver() { + cat > "$WL_SCRATCH/confirm.exp" <<'EOF' +#!/usr/bin/expect -f +# argv: [up args...] +set timeout 1200 +set dr_bin [lindex $argv 0] +set workdir [lindex $argv 1] +set transcript [lindex $argv 2] +set answer [lindex $argv 3] +set up_args [lrange $argv 4 end] + +# The gate must see an interactive run: a real PTY (expect allocates one) and +# no non-interactive signal in the environment. Unset defensively as well as +# in the caller โ€” either override alone is not enough if the other regresses. +catch {unset env(DATAROBOT_CLI_NON_INTERACTIVE)} +set env(DATAROBOT_CLI_FEATURE_WORKLOAD) true + +log_file -noappend $transcript +cd $workdir +eval spawn $dr_bin workload up $up_args + +expect { + -re {Apply this deploy} { + send -- "$answer\r" + exp_continue + } + eof {} + timeout { + puts "\nPTY driver: timed out waiting for the confirm prompt" + exit 124 + } +} + +# Propagate the CLI's exit status: `wait` yields pid spawnid flag status. +foreach {pid spawnid os_error_flag status} [wait] break +exit $status +EOF +} + +# smoke::pty_run [up args...] โ€” runs the PTY driver. +# The caller captures the exit code in PTY_RC itself; this wrapper must never +# abort under set -e (the decline leg legitimately exits nonzero). +smoke::pty_run() { + local transcript=$1 answer=$2 + shift 2 + + PTY_RC=0 + /usr/bin/expect -f "$WL_SCRATCH/confirm.exp" \ + "$DR_BIN" "$(pwd)" "$transcript" "$answer" "$@" || PTY_RC=$? +} + +# --------------------------------------------------------------------------- +# Credential id discovery (read-only). +# +# SMOKE-001's redaction assertion needs one credential-backed env var, and +# SMOKE-003 really applies the manifest โ€” so the reference must name a +# credential that exists. The CLI has no credential subcommand, so the id is +# discovered with a read-only GET on the platform's /credentials/ route using +# the same configured token the CLI itself authenticates with. Only the id is +# read; the stored secret is never fetched and never printed. +# --------------------------------------------------------------------------- +smoke::discover_credential_id() { + local endpoint token override + + endpoint="${DATAROBOT_ENDPOINT:-}" + if [[ -z "$endpoint" ]]; then + endpoint="$(yq -r '.endpoint // ""' "$HOME/.config/datarobot/drconfig.yaml")" + fi + + override="$(wl::resolve_token)" + if [[ -n "$override" ]]; then + token="$override" + else + token="$(yq -r '.token // ""' "$HOME/.config/datarobot/drconfig.yaml")" + fi + + [[ -n "$endpoint" && -n "$token" ]] || return 1 + + curl -fsS --max-time 30 -H "Authorization: Bearer $token" \ + "$endpoint/credentials/?limit=1" \ + | jq -r '.data[0].credentialId // empty' +} + +wl::start_timer "19538: --diff/--confirm live smoke (staging)" + +# --- Deploy the throwaway whoami workload (scenario A pattern) -------------- +work="$WL_SCRATCH/project" +mkdir -p "$work" +cat > "$work/workload.yaml" </dev/null + +# The platform materializes an artifact for the inline image spec; register it +# (if any) so cleanup deletes it after the workload is gone. +wl::dr_capture workload get "$WID" --output-format json +wl::assert_cmd_ok "$WL_RC" "$WL_OUT" "$WL_ERR" "workload get" +AID="$(printf '%s' "$WL_OUT" | jq -r '.artifactId // empty')" +[[ -z "$AID" ]] || wl::register_artifact "$AID" + +# --- Bind and render the manifest ------------------------------------------- +cd "$work" +wl::dr_capture workload config --yes --workload-id "$WID" +wl::assert_cmd_ok "$WL_RC" "$WL_OUT" "$WL_ERR" "workload config --workload-id" +[[ -f .datarobot.yaml ]] || wl::fail "config did not write .datarobot.yaml" +wl::pass "wrote .datarobot.yaml" + +# Baseline: the file the bind rendered is the live state. +wl::dr_capture workload up --dry-run +wl::assert_cmd_ok "$WL_RC" "$WL_OUT" "$WL_ERR" "baseline up --dry-run" +smoke::record "000-baseline-dry-run" + +# --- Edit the manifest: sizing + one env var of each secret shape ----------- +CRED_ID="$(smoke::discover_credential_id)" \ + || wl::fail "cannot discover a credential id (read-only GET on /credentials/ failed)" +if [[ -z "$CRED_ID" ]]; then + wl::fail "no credential visible to this account; the credential-backed env var cannot be referenced" +fi + +# A recognizable fake plaintext so the redaction greps are exact: the value +# must never appear in any diff output, human or JSON. +LIT_SECRET="smoke-literal-secret-${RUN}-never-print" + +yq -i '.runtime.containerGroups[0].replicaCount = 3' .datarobot.yaml +yq -i ".runtime.containerGroups[0].containers[0].environmentVars = [{\"name\": \"SMOKE_LIT_TOKEN\", \"value\": \"$LIT_SECRET\"}, {\"name\": \"SMOKE_API_TOKEN\", \"value\": \"dr-credential:$CRED_ID/apiToken\"}]" .datarobot.yaml +wl::pass "manifest edited: replicaCount 1 -> 3, two env vars added (credential ref + literal)" + +# The reference id is the only credential fact the manifest needs to carry; +# grep for the secret SHAPES below, never for a stored value. +wl::pass "using credential $CRED_ID for the dr-credential reference" + +# --- SMOKE-001: --dry-run --diff shows the sizing change, leaks no values --- +wl::start_timer "SMOKE-001: up --dry-run --diff" + +wl::dr_capture workload up --dry-run --diff +wl::assert_cmd_ok "$WL_RC" "$WL_OUT" "$WL_ERR" "up --dry-run --diff" +smoke::record "001-dry-run-diff" + +printf '%s\n' "$WL_ERR" | grep -Eq '^- .*replicaCount' \ + || wl::fail "SMOKE-001: no '- old' line for the sizing change; stderr was: $WL_ERR" +printf '%s\n' "$WL_ERR" | grep -Eq '^\+ .*replicaCount' \ + || wl::fail "SMOKE-001: no '+ new' line for the sizing change" +wl::pass "diff shows the sizing change as - old / + new" + +# Context either side: the line before the first - marker is context (no marker). +first_change_lineno="$(printf '%s\n' "$WL_ERR" | grep -nE '^[+-] ' | head -1 | cut -d: -f1)" +[[ -n "$first_change_lineno" ]] || wl::fail "SMOKE-001: diff body has no change lines" +prev_lineno=$((first_change_lineno - 1)) +[[ "$prev_lineno" -ge 1 ]] || wl::fail "SMOKE-001: no line before the first change" +prev_line="$(printf '%s\n' "$WL_ERR" | sed -n "${prev_lineno}p")" +if printf '%s' "$prev_line" | grep -qE '^[+-] '; then + wl::fail "SMOKE-001: line preceding the first change is not context: $prev_line" +fi +wl::pass "change is flanked by context lines" + +# Redaction: neither the credential ref material nor the literal value may +# appear; the env var NAME must. (The .err file was persisted above, so the +# absent-checks run against a real file rather than a pipe.) +wl::assert_absent "$TRANSCRIPT_DIR/001-dry-run-diff.err" "$LIT_SECRET" "SMOKE-001 literal secret" +wl::assert_absent "$TRANSCRIPT_DIR/001-dry-run-diff.err" 'dr-credential:' "SMOKE-001 credential ref" +wl::assert_absent "$TRANSCRIPT_DIR/001-dry-run-diff.err" "$CRED_ID" "SMOKE-001 credential id" + +# The env-var change reaches the diff either per element (paths carrying the +# variable name) or as one whole-list row (`environmentVars: [N item(s)]`) +# when the live side has no such list; both spellings must appear as a change +# line, and neither may carry a value. +printf '%s\n' "$WL_ERR" | grep -q 'environmentVars' \ + || wl::fail "SMOKE-001: no environmentVars change line in the diff" +wl::pass "env-var values redacted; the env-var change line is present" + +# (d) No mutation: the workload document is identical around the dry-run. +wl::dr_capture workload get "$WID" --output-format json +wl::assert_cmd_ok "$WL_RC" "$WL_OUT" "$WL_ERR" "workload get (before-diff)" +before="$(printf '%s' "$WL_OUT" | jq -S .)" +wl::dr_capture workload get "$WID" --output-format json +wl::assert_cmd_ok "$WL_RC" "$WL_OUT" "$WL_ERR" "workload get (after-diff)" +after="$(printf '%s' "$WL_OUT" | jq -S .)" +[[ "$before" == "$after" ]] || wl::fail "SMOKE-001: workload document changed around the dry-run diff" +wl::pass "no mutation from --dry-run --diff" + +wl::stop_timer + +# --- SMOKE-002: --confirm answered "n" exits nonzero, mutates nothing ------- +wl::start_timer "SMOKE-002: --confirm decline (PTY)" + +# lib.sh exports DATAROBOT_CLI_NON_INTERACTIVE=1 for every non-PTY leg; the +# gate installs only in an interactive run, so unset it here and keep it unset +# โ€” the PTY driver also unsets it in the child, and only the two overrides +# TOGETHER install the prompt (piped stdin alone suppresses it by design). +unset DATAROBOT_CLI_NON_INTERACTIVE +smoke::write_expect_driver + +wl::dr_capture workload get "$WID" --output-format json +wl::assert_cmd_ok "$WL_RC" "$WL_OUT" "$WL_ERR" "workload get (before-decline)" +before="$(printf '%s' "$WL_OUT" | jq -S .)" + +# Expected NONZERO exit: set +e so the guard cannot abort before SMOKE-003, +# then assert the captured code directly (never wl::assert_cmd_ok here). +set +e +smoke::pty_run "$TRANSCRIPT_DIR/002-decline-pty.log" "n" --confirm +decline_rc=$PTY_RC +set -e + +if [[ "$decline_rc" -eq 0 ]]; then + wl::fail "SMOKE-002: declined run exited 0; the gate must refuse with a nonzero exit" +fi +wl::pass "declined run exited nonzero (rc=$decline_rc)" + +grep -q 'Apply this deploy' "$TRANSCRIPT_DIR/002-decline-pty.log" \ + || wl::fail "SMOKE-002: prompt missing from the PTY transcript" +grep -q '(y/N)' "$TRANSCRIPT_DIR/002-decline-pty.log" \ + || wl::fail "SMOKE-002: prompt is not the y/N (default no) phrasing" +grep -q 'declined: nothing was deployed' "$TRANSCRIPT_DIR/002-decline-pty.log" \ + || wl::fail "SMOKE-002: decline message missing from the PTY transcript" +wl::pass "prompt and decline message present in the PTY transcript" + +# stdout untouched: a declined run prints no endpoint. The workload's own +# endpoint line is what a successful run emits; its absence here, against the +# same PTY driver that shows it on the accept leg below, is the observable. +endpoint="$(printf '%s' "$before" | jq -r '.endpoint // empty')" +if [[ -n "$endpoint" ]] && grep -qF "$endpoint" "$TRANSCRIPT_DIR/002-decline-pty.log"; then + wl::fail "SMOKE-002: declined transcript contains the endpoint (stdout was not untouched)" +fi +wl::pass "declined transcript carries no endpoint (stdout untouched)" + +# (d) Nothing mutated: the workload document matches, and the sizing change is +# still pending exactly as the diff showed it. +wl::dr_capture workload get "$WID" --output-format json +wl::assert_cmd_ok "$WL_RC" "$WL_OUT" "$WL_ERR" "workload get (after-decline)" +after="$(printf '%s' "$WL_OUT" | jq -S .)" +[[ "$before" == "$after" ]] || wl::fail "SMOKE-002: workload document changed after the decline" +wl::dr_capture workload up --dry-run --diff +wl::assert_cmd_ok "$WL_RC" "$WL_OUT" "$WL_ERR" "up --dry-run --diff (pending after decline)" +smoke::record "002-pending-after-decline" +printf '%s\n' "$WL_ERR" | grep -Eq '^\+ .*replicaCount.*3' \ + || wl::fail "SMOKE-002: sizing change no longer pending after the decline (spec mutated?)" +wl::pass "decline mutated nothing; sizing change still pending" + +wl::stop_timer + +# --- SMOKE-005: --diff JSON envelope with the change still pending ---------- +wl::start_timer "SMOKE-005: --diff --output-format json envelope" + +wl::dr_capture workload up --dry-run --diff --output-format json +wl::assert_cmd_ok "$WL_RC" "$WL_OUT" "$WL_ERR" "up --dry-run --diff --output-format json" +smoke::record "005-json-diff" +printf '%s' "$WL_OUT" > "$TRANSCRIPT_DIR/005-json-diff.raw" + +# (a) Exactly one JSON document on stdout. The envelope nests the plan under +# the "up" key (the deploy's outcome object), hence .up.plan.diff. +docs="$(printf '%s' "$WL_OUT" | jq -s 'length')" +[[ "$docs" == "1" ]] || wl::fail "SMOKE-005: stdout carries $docs documents, expected 1" +wl::pass "stdout is exactly one JSON document" + +# (b) A non-empty changes list that names the sizing field with live/want. +changes="$(printf '%s' "$WL_OUT" | jq '.up.plan.diff.changes | length')" +[[ "$changes" -gt 0 ]] || wl::fail "SMOKE-005: plan.diff.changes is empty" +sizing_change="$(printf '%s' "$WL_OUT" \ + | jq -c '[.up.plan.diff.changes[] | select(.path | contains("replicaCount"))][0]')" +[[ "$sizing_change" != "null" ]] || wl::fail "SMOKE-005: no replicaCount entry in plan.diff.changes" +have="$(printf '%s' "$sizing_change" | jq '.have')" +want="$(printf '%s' "$sizing_change" | jq '.want')" +[[ "$have" == "1" && "$want" == "3" ]] \ + || wl::fail "SMOKE-005: sizing change is have=$have want=$want, expected 1 -> 3" +wl::pass "plan.diff.changes carries the sizing change (have=1, want=3)" + +# (c) Env-var paths are redacted in the envelope. A whole-list env-var change +# arrives as one row whose own path only ends in .environmentVars, so the +# envelope marks it `redacted: true` and keeps the structure with the variable +# NAMES but not the values (scrubEnvList) โ€” asserting null have/want here +# would test the wrong shape. The hard property is the whole-document scan +# below: neither the literal secret nor any credential material appears. +printf '%s' "$WL_OUT" | jq -e ' + [.up.plan.diff.changes[] | select(.path | contains("environmentVars"))] | length > 0 +' >/dev/null || wl::fail "SMOKE-005: no environmentVars entries in plan.diff.changes" +printf '%s' "$WL_OUT" | jq -e ' + [.up.plan.diff.changes[] + | select(.path | contains("environmentVars")) + | .redacted == true] | all +' >/dev/null || wl::fail "SMOKE-005: an environmentVars change is not redacted in the envelope" +wl::assert_absent "$TRANSCRIPT_DIR/005-json-diff.raw" "$LIT_SECRET" "SMOKE-005 literal secret" +wl::assert_absent "$TRANSCRIPT_DIR/005-json-diff.raw" 'dr-credential:' "SMOKE-005 credential ref" +wl::assert_absent "$TRANSCRIPT_DIR/005-json-diff.raw" "$CRED_ID" "SMOKE-005 credential id" +wl::pass "env-var values redacted from the JSON envelope" + +# (d) unmanaged is present (array, possibly empty). +printf '%s' "$WL_OUT" | jq -e '.up.plan.diff.unmanaged | type == "array"' >/dev/null \ + || wl::fail "SMOKE-005: plan.diff.unmanaged missing or not an array" +wl::pass "plan.diff.unmanaged present" + +wl::stop_timer + +# --- SMOKE-003: --confirm answered "y" applies the sizing change ------------ +wl::start_timer "SMOKE-003: --confirm accept (PTY)" + +# The env vars were diff surface only. The settings update sends the runtime +# block verbatim and the platform rejects environmentVars under runtime +# containers with a 422 ("Extra inputs are not permitted"), so a manifest +# that asks for runtime env vars can never be applied -- the two variables +# come back out here, and the accepted deploy carries the sizing change, the +# change the confirm gate exists to gate. (Pre-existing CLI/platform mismatch +# in how runtime env vars apply, found by this smoke and reported separately; +# it does not touch the diff, the confirm gate, or the JSON envelope.) +yq -i 'del(.runtime.containerGroups[0].containers[0].environmentVars)' .datarobot.yaml +wl::pass "env vars removed from the runtime block; the sizing change stays pending" + +smoke::pty_run "$TRANSCRIPT_DIR/003-accept-pty.log" "y" --confirm +wl::assert_cmd_ok "$PTY_RC" "see transcript" "$(tail -40 "$TRANSCRIPT_DIR/003-accept-pty.log")" \ + "up --confirm answered y" +wl::pass "accepted run exited 0" + +grep -q 'Apply this deploy' "$TRANSCRIPT_DIR/003-accept-pty.log" \ + || wl::fail "SMOKE-003: prompt missing from the PTY transcript" +wl::pass "prompt preceded the apply in the PTY transcript" + +# (b) The sizing change was applied. `workload get` does not surface sizing, +# so the CLI's own live-vs-manifest verdict is the observable: with the +# manifest still asking for replicaCount 3, an up-to-date dry-run proves the +# live runtime now matches the manifest's new value. +wl::dr_capture workload up --dry-run +wl::assert_cmd_ok "$WL_RC" "$WL_OUT" "$WL_ERR" "up --dry-run (after accept)" +smoke::record "003-dry-run-after-accept" +printf '%s\n' "$WL_ERR" | grep -q 'Already up to date' \ + || wl::fail "SMOKE-003: workload not up to date after the accepted deploy; stderr was: $WL_ERR" +wl::pass "live runtime now matches the manifest (up-to-date verdict)" + +# (d) stdout carries the endpoint: the accepted PTY transcript shows the +# endpoint line the declined one deliberately lacked. +wl::dr_capture workload get "$WID" --output-format json +wl::assert_cmd_ok "$WL_RC" "$WL_OUT" "$WL_ERR" "workload get (after-accept)" +endpoint="$(printf '%s' "$WL_OUT" | jq -r '.endpoint // empty')" +[[ -n "$endpoint" ]] || wl::fail "SMOKE-003: workload has no endpoint" +grep -qF "$endpoint" "$TRANSCRIPT_DIR/003-accept-pty.log" \ + || wl::fail "SMOKE-003: accepted transcript does not carry the endpoint" +wl::pass "accepted transcript carries the endpoint" + +wl::wait_for_status "$WID" running 300 >/dev/null + +wl::stop_timer + +# --- SMOKE-004: cleanup leaves no RUN-prefixed resources -------------------- +wl::start_timer "SMOKE-004: cleanup" + +# Run the same cleanup the EXIT trap would run, while the script can still +# assert on the result; the trap runs again on exit as a verified no-op. +smoke::cleanup_resources +wl::pass "cleanup deleted workload $WID (and artifact ${AID:-none})" + +wl::dr_capture workload list --output-format json +wl::assert_cmd_ok "$WL_RC" "$WL_OUT" "$WL_ERR" "workload list (post-cleanup)" +smoke::record "004-workload-list-post-cleanup" +leftover_wl="$(printf '%s' "$WL_OUT" \ + | jq -r --arg run "$RUN" '[.workloads[]? | select(.name | contains($run))] | length')" +[[ "$leftover_wl" == "0" ]] \ + || wl::fail "SMOKE-004: $leftover_wl RUN-prefixed workload(s) remain on staging" +wl::pass "no RUN-prefixed workloads remain (dr workload list)" + +if [[ -n "$AID" ]]; then + wl::dr_capture artifact get "$AID" --output-format json + if [[ "$WL_RC" -eq 0 ]]; then + wl::fail "SMOKE-004: artifact $AID still exists after cleanup" + fi + wl::pass "artifact $AID deleted (get fails as expected)" +fi + +wl::stop_timer + +wl::stop_timer +echo "โœ… RAPTOR-19538 diff/confirm smoke passed โ€” transcripts in $TRANSCRIPT_DIR" diff --git a/smoke_test_scripts/workload/TICKETS.md b/smoke_test_scripts/workload/TICKETS.md index 92b680e75..6e0701513 100644 --- a/smoke_test_scripts/workload/TICKETS.md +++ b/smoke_test_scripts/workload/TICKETS.md @@ -12,6 +12,7 @@ scenario script (grep with `rg '^# Ticket:' smoke_test_scripts/workload`). | C โ€” re-bind tuned | `RAPTOR-19533-C-rebind.sh` | RAPTOR-19533 | re-bind preserves live tuning; FileExists guard; delete-rebind restore | | D โ€” built-workload rebuild | `RAPTOR-19533-D-built.sh` | RAPTOR-19533 | no ErrImagePull after re-bind; platform rebuilds from imageBuildConfig | | Artifact lifecycle | `artifact-lifecycle.sh` | none โ€” basic acceptance | `dr artifact` create/get/list/code sync/versions/del CLI-side state | +| diff/confirm live smoke | `RAPTOR-19538-diff-confirm.sh` | RAPTOR-19538 | staging: `--dry-run --diff` redaction, `--confirm` decline/accept (PTY), JSON diff envelope, cleanup. Run directly: interactive PTY + staging writes | ## Adding a scenario for a new ticket