[RAPTOR-19538] feat(workload): --diff unified plan rendering and --confirm opt-in gate for dr workload up - #880
Draft
ajalon1 wants to merge 12 commits into
Draft
Conversation
New internal/uidiff package: an ordered row list in, a unified diff out.
It owns only what is generic about that rendering, so the up command's
diff mode (and later a component/template adapter) can stay thin.
- Row model: Kind (Context | Add | Del | Unmanaged), Path for the
redaction hook, Text pre-formatted by the caller.
- Render(w, rows, opts) with a fixed default 3-line context window
(opts.Context, zero means 3; deliberately no CLI flag), collapsing
each run of Context/Unmanaged rows longer than the window into a
single "... N identical lines" that counts exactly the rows it hides,
correct at both ends of the list.
- Styling reuses the tui palette so a diff and the default plan read as
one visual language: SuccessStyle for "+", WarnStyle for "-" and the
caller's unmanaged marker, HintStyle for context and collapse lines.
- Caller-supplied Redact hook suppresses the value portion of every Kind
it fires for; redacted rows render a placeholder built from the path
("set" / "changed" / "(redacted)"), never the value.
- CODEOWNERS: internal/uidiff/ routes to @datarobot-oss/workload-cli,
keeping the package owned beside the code that will consume it first.
The package imports nothing under internal/workload (verified via
go list -deps); the one-way dependency up -> uidiff is the point.
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…de Subset --diff needs the leaves that already agree as context, not just the ones that differ. DiffRows(want, have) emits one row per leaf of want and marks each 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 lacks 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 stays Extra's question; the caller merges the two. Subset and Extra are untouched, so the default plan keeps its exact shape.
RenderDiff writes the plan block for --diff: a changed leaf states both sides of itself as - old / + new, agreeing leaves are context that collapses past the three-line window, and nothing is truncated -- the default plan caps its detail list because it is a summary, a diff is the detail. Unmanaged live fields are counted once as "N fields not managed by this file" and never render as removals, and the verdict plus that count still print for an empty plan, where the default mode short-circuits. Build now computes the rows the diff draws: the spec half and the runtime half from the same walks that produce Artifact and Runtime, with the unchanged leaves kept as context; the create path walked against nil so a first deploy is all additions; and the two synthetic changes the walk cannot see (artifactId, artifact.type) merged in, because a change the default plan prints must not be invisible in the diff. Extra's unmanaged paths land on the plan deduplicated across the two halves, since one element unmanaged in both documents is one field to a reader. Redaction is the uidiff hook fed redacted(): no value behind an .environmentVars[ path prints in any line state, while the variable names stay visible. The state, lock, code and artifact entry lines render as action lines in their existing positions; the artifact entry stays because "a new version will be minted" is not visible in any field value. A first deploy gets a header saying so, the create summary line the default plan prints, and an all-additions body. Render itself is untouched; a captured-baseline regression test pins the default output byte for byte. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…e drift Code drift in --diff mode printed the same bare "~ code N files changed" count the default plan prints, which is exactly the summary a diff exists to replace. CodeChange now carries the dry-run SyncPlan that defaultCodeChange already computed and threw away, and RenderDiff feeds it to the sync command's own display.PrintPlan -- the renderer `dr artifact code sync --dry-run` uses -- so both commands describe one upload with one format, and the diff names the files instead of counting them. The list keeps the code block's position, ahead of the lock and artifact lines. A first deploy has no plan to list, because nothing was ever uploaded to compare against, so it keeps the all-files wording. A plan measured without its list, which only a test harness wiring the count alone can produce, falls back to the count rather than going silent about drift. Render is untouched: the default mode keeps the bare count byte for byte. The seam is unchanged -- codeChangeFn still hands back a CodeChange, and the plan is data rather than engine state, so it survives the Close that releases the project lock. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
up.Options gains Diff and Run picks RenderDiff over Render at the one render call site, so --diff swaps 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. The shell registers the flag with help text that says the default plan is the summary and the diff is the detail, threads it into the options, and reports it to telemetry under its own key so adoption is readable without guessing from dry_run. Proved by seam tests at both layers: stubRun observes the option, a sized fixture shows the selector swapping the renderer while the default path keeps its have -> want summary with no hunks, and --dry-run --diff prints the identical diff body the wet run prints, with the mutating seams wired to fail the test on the dry leg. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…elope
With --diff the plan object gains a structured `diff` section: one
entry per changing leaf ({path, have, want, absent}) in the order the
diff body prints them -- artifact rows, then the synthetic artifactId
and artifact.type entries, then runtime -- plus the unmanaged path
list, so a consumer reads the same changes as data that the human
block prints as lines. Entries on an environmentVars path serialise
with their values withheld and a redacted marker, the JSON twin of
the rule that keeps secrets out of the printed plan: the envelope is
the artefact that ends up in CI logs, so redaction happens before
marshalling rather than after.
The section is present only when --diff was asked for: JSON() keeps
its exact shape, a run without the flag gains no diff key, and the
legacy artifact/runtime arrays ride along unchanged beside it. The
dry run and the run that follows it carry the same section, because
both project the one pre-apply plan.
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…ally changing diffActionLines handed plan.Code to codeBlock unconditionally, so a plan whose drift was elsewhere -- runtime sizing, say, with a tree matching the last deploy -- grew a stray "~ code 0 files changed since the last deploy" line naming a sync the run was never going to perform. The block now exists only when Code.Changed(). The same fix settles which predicate gates the file list. IsEmpty() counts Downloads and Conflicts, which a deploy never pulls, so a plan holding only those would have rendered a file list for a sync that will not happen; the fallback now keys on len(Uploads)+len(Deletes), the same count Files carries, and the block is documented as one predicate throughout.
… JSON diff rows Path-based redaction in changeJSON only caught rows whose own path sits inside an environmentVars list. But DiffRows emits a whole-element row for every NEW name-keyed list element: a new container is one row whose path names the container, with the entire container map as want, so its environmentVars block (plaintext literals and dr-credential refs alike) serialised raw into plan.diff.changes. Rather than dropping the whole entry, which would hide the element's other fields from a consumer that has to review the plan, the subtree is scrubbed before it marshals: variable names survive, every value does not, the copy never mutates the rows the human diff still reads, and the entry carries the redacted marker. The regression pins a new-container fixture carrying both a credential ref and a plaintext value, asserting neither reaches the serialised envelope. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
An interactive run asked with --confirm now prints the plan (or the diff), then asks '? Apply this deploy? (y/N)' on stderr and deploys only on an affirmative; anything else, including an empty answer, declines, and the run returns up.ErrDeclined having touched nothing. The gate fires whenever the run would otherwise mutate -- a pending plan, or an empty one with --lock waiting to make the serving artifact permanent -- and never on a dry run or a wholly empty one, because unmanaged fields are never mutated either. The question is installed only when stdin is a terminal and no non-interactive signal is set (--yes, --output-format json, DATAROBOT_CLI_NON_INTERACTIVE): suppression composes, so there is deliberately no cobra mutual exclusivity, and the flag help documents the matrix. The locked-production typed confirm still fires after an accepted y/N, asking its own question. Both flags are reported to telemetry under their own keys, and the command Long names --dry-run --diff as the look-without-touching combination. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
… the envVars list
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 changed leaf whose path ENDS at the list. redacted() only matches the
per-variable spelling (".environmentVars["), and the container-level scrub
only saw env-var blocks nested BELOW a row's value, so this row serialized
every variable's literal plaintext and credential ref straight into the
JSON envelope. Caught by the m3-live-smoke staging run: the human diff was
safe (format() summarizes composites), the machine envelope was not.
The constructor now names the shape itself (endsAtEnvVars) and scrubs the
list directly, keeping the variable names and dropping every value, marked
redacted like the other refusal paths.
…nfirm RAPTOR-19538-diff-confirm.sh walks the six VAL-SMOKE assertions against staging on a RUN-identified throwaway whoami workload: the sizing change shows as a unified diff under --dry-run --diff with neither env-var value leaking, the JSON envelope carries the structured change list with the values scrubbed, a --confirm decline exits nonzero through a real PTY (DATAROBOT_CLI_NON_INTERACTIVE unset by the scenario; the expect driver allocates the terminal) having mutated nothing, an accepted y applies the sizing, and the trap cleanup deletes the workload and artifact so no RUN-prefixed resource stays on staging. Two shapes the fixtures never showed: the env-var change arrives as one whole-list row when the live side has no list to match against, and the settings API rejects environmentVars under runtime containers with a 422, so the smoke drops the two variables before the accepted deploy and keeps them as diff surface for the redaction assertions. Registered in TICKETS.md; deliberately not wired into run_workload_smoke_test.sh because it drives an interactive PTY prompt and writes to staging.
|
🎫 Jira: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
RATIONALE
dr workload uptoday prints the deploy plan as apath: have -> wantlist capped at 6 entries, which hides most of what a deploy will change on a large manifest, and then applies it immediately with no checkpoint. This change adds two opt-in flags:--diffrenders the deploy plan as a unified diff (context lines, collapsed identical runs, no truncation), and--confirmasks a y/N question on stderr before anything is applied. The unified-diff primitives live in a new shared packageinternal/uidiffso future consumers (dr component update, template regeneration) can adopt the same show-a-diff-then-confirm UX; this PR only wiresupto it.CHANGES
Milestone 1 - shared diff primitives + row walker
internal/uidiffpackage: generic unified-diff primitives (row model, fixed 3-line context window, collapse of long unchanged runs into... N identical lines, line styling reusing thetuistyles, caller-supplied redaction hook). It has no dependency oninternal/workload/...and is routed to@datarobot-oss/workload-clivia a.github/CODEOWNERSentry.DiffRows(want, have)ininternal/workload/up: a sibling walker toSubsetthat emits a row for every leaf ofwant, with the same name-keyed list matching (containerGroups/containers/environmentVarsbyname), the same memory-string tolerance, and sorted key ordering.waitstub ininternal/workload/up/run_test.goso the full tree lints and tests green.Milestone 2 -
--diffflag + JSON envelopeRenderDiffininternal/workload/up: three-state rendering (changing =- old/+ new; unchanged = collapsible context; unmanaged = marked context or anN fields not managed by this filesummary, never a removal), a first-deploy all-additions header, and env-var values redacted at every layer (human diff, context lines, JSON). The defaultRenderpath is untouched, so output without--diffis unchanged and the 6-entry truncation stays.internal/workload/sync/display.PrintPlanrenderer (thedr artifact code sync --dry-runformat), not a second file-list format.--diffis registered ondr workload up, threaded throughup.Options, and added totelemetry.TrackWith.plan.diffsection (changesas structured{path, have, want, absent}rows with values redacted before serialisation, plusunmanaged), while the legacyartifact/runtimestring arrays are kept unchanged for backwards compatibility.CHANGELOG.mdentry under[Unreleased].Milestone 3 -
--confirmgate + live staging smoke--confirmasks? Apply this deploy? (y/N)on stderr after the plan renders and before any mutating path; declining exits nonzero having mutated nothing;--dry-runnever prompts. Under any non-interactive trigger (--yes,-o json,DATAROBOT_CLI_NON_INTERACTIVE, piped stdin) the flag is silently suppressed and behaves as if not given (suppression-over-error, the CLI-wide convention; no cobra mutual exclusivity is added). The locked-production typed confirm still applies on top.telemetry.TrackWith; the cobraLonghelp documents what--diffprints, what--confirmasks, and that--dry-run --diffis the look-without-touching combination.smoke_test_scripts/workload/RAPTOR-19538-diff-confirm.sh): deploy a throwaway workload, change sizing, verifyup --dry-run --diffoutput, then a real run with--confirmanswering "n" to verify the nonzero exit and that nothing was mutated; trap-based cleanup of the run-identified throwaway resources.NOTES
docs/commands/workload.mdis deliberately untouched by this PR: that doc is owned by RAPTOR-18971, and flag documentation for this change lives in cobra help and the CHANGELOG only.plan.diffsection in the JSON output; the legacy arrays are kept so existing consumers keep parsing today's shape.PR Automation
Comment-Commands: Trigger CI by commenting on the PR:
/trigger-smoke-testor/trigger-test-smoke- Run smoke tests/trigger-install-testor/trigger-test-install- Run installation testsLabels: Apply labels to trigger workflows:
run-smoke-testsorgo- Run smoke tests on demand (only works for non-forked PRs)Important
For Forked PRs: The
run-smoke-testslabel won't work. A required Smoke Tests check will block merge until a maintainer acts:/approve-smoke-teststo run smoke tests (results will set the check)/skip-smoke-teststo bypass the check without running testsPlease comment requesting a maintainer review if you need smoke tests to run.