bundle deploy/destroy: report per-resource actions and a summary - #5720
Open
denik wants to merge 28 commits into
Open
bundle deploy/destroy: report per-resource actions and a summary#5720denik wants to merge 28 commits into
denik wants to merge 28 commits into
Conversation
Collaborator
Integration test reportCommit: 3fce466
10 interesting tests: 4 SKIP, 3 flaky, 3 RECOVERED
Top 30 slowest tests (at least 2 minutes):
|
denik
force-pushed
the
denik/new-deploy-output
branch
from
June 25, 2026 14:04
2e8f647 to
5d6d622
Compare
denik
force-pushed
the
denik/new-deploy-output
branch
from
July 23, 2026 08:51
5d6d622 to
139c45e
Compare
denik
marked this pull request as ready for review
July 23, 2026 14:12
denik
force-pushed
the
denik/new-deploy-output
branch
3 times, most recently
from
July 30, 2026 11:13
c56ed07 to
f8baedd
Compare
denik
force-pushed
the
denik/new-deploy-output
branch
3 times, most recently
from
August 9, 2026 11:42
d88ce7f to
fd2b017
Compare
lennartkats-db
requested changes
Aug 11, 2026
denik
force-pushed
the
denik/new-deploy-output
branch
2 times, most recently
from
August 12, 2026 10:39
4bee1f1 to
74bd52e
Compare
denik
force-pushed
the
denik/new-deploy-output
branch
from
August 13, 2026 11:55
74bd52e to
d1e61ac
Compare
| @@ -40,6 +41,7 @@ See https://docs.databricks.com/en/dev-tools/bundles/index.html for more informa | |||
| cmd.Flags().BoolVar(&autoApprove, "auto-approve", false, "Skip interactive approvals that might be required for deployment.") | |||
| cmd.Flags().MarkDeprecated("compute-id", "use --cluster-id instead") | |||
| cmd.Flags().BoolVar(&verbose, "verbose", false, "Enable verbose output.") | |||
| cmd.Flags().BoolVarP(&quiet, "quiet", "q", false, "Only print the summary line, not the per-resource actions.") | |||
Contributor
There was a problem hiding this comment.
The PR description says repeating the flag (-qq) prints "only warnings and errors, which also drops progress messages (Uploading bundle files to ..., Building ..., Executing 'postdeploy' script)". But --quiet is a plain bool here (and in plan.go / pipelines/deploy.go), and b.Quiet in bundle.go is a bool whose only consumer is the if !b.Quiet guard in logDeploySummary, which gates just the per-resource lines.
pflag parses -qq as setting the bool twice, so -qq is identical to -q. Running bundle deploy -qq against a live workspace printed the Uploading bundle files to ... progress line and the full Files:/Resources: summary, byte-for-byte the same as -q (a changed resource still showed everything except the per-resource line). The second quiet level described in the PR body isn't implemented — either trim the description, or make this a repeat counter (e.g. CountVarP) and wire progress suppression through cmdio.
This review comment was added by Isaac.
Contributor
Author
There was a problem hiding this comment.
Right, BoolVarP made -qq identical to -q. Now a CountVarP on deploy/plan/destroy/pipelines deploy (destroy had no -q before), with progress suppression through cmdio.WithQuiet/LogProgress as you suggested; LogString is untouched since it also carries results.
acceptance/bundle/quiet-levels asserts all nine command/level combinations, and shared-root-path / create-error show -qq still printing warnings and errors.
One exception: -qq hides destroy's deletion listing only with --auto-approve; without it we are about to ask for consent, so it always prints (TestApprovalForDestroyQuietWhilePrompting — needs a TTY, so not an acceptance test).
This reply was added by Claude Code.
| if plan.NotSelected > 0 { | ||
| summary += fmt.Sprintf(", %d not selected", plan.NotSelected) | ||
| } | ||
| cmdio.LogString(ctx, summary+".") |
Contributor
There was a problem hiding this comment.
This Resources: ... unchanged. summary line ends with a period (summary+"."), which is inconsistent with the sibling Files: %d uploaded, %d deleted line just above it (line 151, no period) and with the Plan: %d to add, ... %d unchanged line in cmd/bundle/plan.go (also no period). Within this PR, Resources: and Destroy: %d deleted. end with a period while Files: and Plan: don't — worth making the status lines uniform one way or the other.
This review comment was added by Isaac.
Contributor
Author
There was a problem hiding this comment.
Agreed — dropped from Files:, Resources: and Destroy: so all four status lines match Plan:. The same change removes the conditions that hid these lines, so the output shape no longer varies between deploys, which also covers the blank-line point from your first review.
This reply was added by Claude Code.
denik
force-pushed
the
denik/new-deploy-output
branch
from
August 13, 2026 14:48
3336d7b to
50eb18d
Compare
`bundle deploy` now lists the per-resource actions and a summary line,
mirroring `bundle plan`, instead of generic progress chatter:
created jobs.foo
deleted pipelines.bar
Deploy: 1 created, 0 changed, 1 deleted, 3 unchanged.
`bundle destroy` gets a matching summary line (`Destroy: N deleted.`),
counting top-level resources to match its approval list.
Details:
- Add CountActions()/ActionCounts and NotSelected to deployplan.Plan.
- Add --quiet/-q to plan, deploy and pipelines deploy to print only the
summary line.
- With --select, the summary appends ", N not selected".
- Drop progress chatter: "Deploying resources...", "Deployment complete!",
"Updating deployment state...", "Deleting files...", "Destroy complete!".
A few acceptance tests where the engines genuinely diverge per-resource
(secret-scope/grant/permission sub-resources, cluster resize verb, no-op
update detection) use `deploy -q | grep -v '^Deploy:'` to stay engine-stable.
Co-authored-by: Isaac
The rebase landed the NotSelected assignment inside enqueueReachable instead of at the end of FilterToSelected. Move it back so it runs once after the BFS prune completes. Co-authored-by: Isaac
The rebase pulled in many acceptance tests added on main after this branch point (postgres_*, genie_spaces, migrate variants, etc.) that exercise deploy/destroy output. Regenerate their goldens for the new per-resource-action + summary format. Also normalize the Destroy: count in the delete-trashed-out-of-band dashboard test via a Repls entry: terraform refreshes the trashed dashboard away and reports "0 deleted" while direct still counts it, so the count diverges per engine. Co-authored-by: Isaac
mlops-stacks is a Cloud-only test (Local=false), so ./task test-update cannot regenerate it locally. Hand-update its golden to the new per-resource-action + summary format: 4 fresh creates and a matching Destroy: 4 deleted line. Resource order matches the destroy list already in the golden. This was the only integration-test failure after the deploy/destroy output change. Co-authored-by: Isaac
Regenerated against AWS cloud via deco. The prior hand-edit undercounted: each resource also deploys a .permissions or .grants sub-node, so deploy reports 8 created, not 4. Destroy still counts the 4 top-level resources. Co-authored-by: Isaac
Co-authored-by: Isaac
Co-authored-by: Isaac
- Print the deploy summary after state/metadata upload instead of before PushResourcesState, so a state-push failure is not masked by a success summary (matches the old "Deployment complete!" placement). - Gate the ", N not selected" suffix on plan.NotSelected instead of b.Select, so it survives a deploy from a --plan file (where --select was applied at plan time and b.Select is empty). NotSelected is only ever set by FilterToSelected, so it stays 0 when --select is unused. Co-authored-by: Isaac
- select/plan_file_survives_not_selected: deploy from a --plan file without re-passing --select still prints ", 2 not selected", proving the suffix comes from the serialized plan.NotSelected. Fails on the old len(b.Select) gating. - deploy/summary-suppressed-on-state-push-failure: inject a 403 on the resource-state upload; the deploy summary must not print (the resources are created remotely but state push fails). The cleanup deploy has no fault and does print the summary. Fails if the summary is logged before PushResourcesState. Co-authored-by: Isaac
Two tests merged on main after this branch's base still had the old deploy/destroy progress strings: - empty_string_dropped: regenerated to the per-resource + summary format. - grants/schemas/all_privileges_coexist: the second deploy diverges per engine (terraform revokes the extra out-of-band privilege and reports "1 changed"; direct sees no drift), so filter its summary with deploy -q | grep -v '^Deploy:' to stay engine-stable. Co-authored-by: Isaac
…esources..." The WAL crash tests normalize a Windows process-kill (which exits 1 with no bash "Killed" message) into the [PROCESS_KILLED] / Exit code: [KILLED] markers via a Repls rule. That rule anchored on the "Deploying resources..." line, which this branch removed, so on Windows the raw "Exit code: 1" leaked through and chain-3-jobs / crash-after-create failed deterministically. Re-anchor the rule on the "Uploading bundle files" line, which the kill now immediately follows (a genuine error exit prints an "Error:" line in between, so it stays untouched). Co-authored-by: Isaac
ai_runtime_code_source, summary-suppressed-on-state-push-failure, force-lock-node-limit, webhook-reorder-remote and telemetry/deploy landed on main with the old progress output; regenerate them to the per-resource + summary format. shell/cmd is Windows-only (cmd.exe artifact build) so its golden is updated by hand to match its bash/sh siblings. Co-authored-by: Isaac
The summary was printed inside deployCore, so a failing postdeploy script reported its error after a "Deploy: ..." line that appeared to vouch for the whole run. Move the summary after the script. It still prints when the script fails -- the resources were applied by then, so the counts are accurate -- and the script's error still propagates (exit code 1). Earlier failures return without a summary, since the plan counts would describe what was intended rather than what was applied. Also: - pipelines deploy --quiet now suppresses the per-resource "View your ... here" lines, matching bundle deploy; covered by create-pipeline. - CountActions iterates the plan map directly (no needless sort). - Document why the destroy count includes Gone resources: they are excluded from the approval prompt as non-destructive, but destroying them does remove them from state, and bundle deploy likewise counts them as deleted. Co-authored-by: Isaac
…table Tests added on main still had the old progress output; regenerate them to the per-resource + summary format. In grants/schemas/remove_all the engines classify emptying the grants list differently (terraform reports the grants node as deleted, direct as updated), so that deploy uses "-q | grep -v '^Deploy:'" to stay engine-stable, matching all_privileges_coexist. Co-authored-by: Isaac
Co-authored-by: Isaac
Co-authored-by: Isaac
…:, capitalize verbs
Reviewer feedback on the deploy summary:
- The blank line before the summary only appeared when there were per-resource
lines, so the output shape varied between deploys. Dropped.
- "0 changed" looked like a no-op when only business logic (a .py or .sql file)
changed. Report file sync separately as "Files: N uploaded, N deleted", printed
only when something was synced. libs/sync now keeps the counts from its diff;
b.Files lists everything tracked, not what changed.
- Per-resource verbs are capitalized ("Created jobs.foo") to match the sentence
case of the surrounding output. "bundle plan" keeps the lower-case present
tense, so the two commands stay distinguishable.
The summary line is now "Resources:" rather than "Deploy:", since it counts
resources and sits next to a "Files:" line. It is omitted entirely when the plan
is empty. Tests that filtered '^Deploy:' to hide engine-divergent counts now
filter '^Resources:' and let the engine-independent Files: line through.
The new "Files: N uploaded, N deleted" line asserts how many files the deploy synced, which surfaced two fixtures whose synced file set was not reproducible: - bundle/python: requirements-latest.txt was created only for PYDAB_VERSION=current, so that variant synced one more file than the pinned one while both share a single output.txt. Create it in both branches. - bundle/apps/job_permissions: the script writes out.after_first_deploy.* between deploys, so a later deploy's count depended on which engine variant ran before. Exclude out.* from sync.
The test is gated to Windows (it runs cmd.exe), so -update on macOS/Linux cannot refresh it. The new value is taken from its bash/sh/default siblings, which have an identical file set and now all record "Files: 6 uploaded, 0 deleted".
--quiet becomes repeatable: -q prints only the summary lines, -qq additionally
drops them along with progress messages ("Uploading bundle files to ...",
"Building ...", "Executing 'postdeploy' script"). Warnings and errors are never
suppressed: they go through libs/logdiag, not cmdio.
Progress messages are emitted by mutators that receive only a context, not the
bundle, so the level travels on the context via cmdio.WithQuiet/LogProgress.
LogString is left alone, since it also carries results the user asked for.
bundle destroy gains -q; plan/deploy/pipelines deploy switch their bool flag to
a count. Also drops the trailing period from the summary lines so Files:,
Resources: and Destroy: punctuate alike.
The grep pipelines existed to hide summary lines that differ between engines. -qq does it directly, so the intent is visible at the call site. The two clusters/deploy tests keep -q: only their per-resource verb diverges (direct reports resize, terraform update), and the Resources: summary agrees, so it is still worth asserting. Documented in each script.
Drop the conditions that hid "Files:" when nothing synced and "Resources:" when the plan was empty, so the output has the same shape on every deploy. delete-trashed-out-of-band drops its Repls rule for the divergent destroy count and passes -qq instead, with the reason stated in the script.
destroy -qq still printed the deletion listing and the root-path line, so it did not match deploy -qq. It is now silent -- but only with --auto-approve: without it we are about to ask for consent and the user must see what they are consenting to, so the listing prints at any -q level. logPipelineDeleteApproval still runs when quiet, since its cascade lookup can fail and that error has to surface; only its printing is suppressed. acceptance/bundle/deploy/quiet-levels now asserts all nine combinations of plan/deploy/destroy x default/-q/-qq. The prompting case needs a TTY, which acceptance tests do not have, so it is covered by a unit test.
Each level now starts from nothing deployed, with a destroy in between, so the three outputs differ only by the flag. Before, the second deploy had nothing left to create and the third nothing to change, so the counts differed for that reason instead of because of -q. Deploy an experiment rather than a notebook job: no local files to sync, no language runtime leaving __pycache__ behind, and one plan node on both engines (secret scopes model permissions as a plan node on direct but not terraform, which made the counts diverge).
The direct-only migration tests from #6261 landed on main while this branch was in review, so their goldens still carried the old "Deploying resources..." output.
The files are uploaded before planning and applying, so a deploy that fails later still synced them and "Files:" is accurate. It is now printed on those paths too, via a defer armed right after the upload so a new early return cannot drop it. "Resources:" stays absent there: a failure before planning has no counts to report, and one after it would describe what was intended rather than what was applied. Renamed the test that asserts this to partial-summary-on-push-fail. jobs/shared-root-path and jobs/create-error now deploy at each quiet level, showing that -q and -qq still print warnings and errors.
denik
force-pushed
the
denik/new-deploy-output
branch
from
August 13, 2026 17:22
50eb18d to
3fce466
Compare
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
3 participants
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.
bundle deploynow lists the per-resource actions and summary lines, instead of generic progress chatter:Files:is reported separately because a deploy that only changes business logic (a.pyor.sqlfile) leaves every resource unchanged, so a lone0 changedreads like a no-op. Both lines always print, so the shape of the output does not vary between deploys.A deploy that fails after uploading still reports
Files:— those files were uploaded. It does not reportResources:, which would describe what was intended rather than what was applied.bundle destroygets a matching summary line (Destroy: N deleted), counting top-level resources to match its approval list.Details
CountActions()/ActionCountsandNotSelectedtodeployplan.Plan.libs/syncretains the upload/delete counts from its diff;b.Fileslists every tracked file, not the changed ones.-q/--quietprints only the summary lines. Repeat it (-qq) to print only warnings and errors, which also drops progress messages (Uploading bundle files to ...,Building ...,Executing 'postdeploy' script). Supported onplan,deploy,destroyandpipelines deploy;destroydid not have-qbefore. It is a repeat counter, and progress suppression travels on the context (cmdio.WithQuiet/LogProgress) because those messages come from mutators that receive no bundle.acceptance/bundle/quiet-levelscovers all nine command/level combinations; acceptance tests use-qqwhere the engines genuinely diverge per-resource.--select, the resource summary appends, N not selected.Deploying resources...,Deployment complete!,Updating deployment state...,Deleting files...,Destroy complete!.postdeployscript, so a failing script is not preceded by a line that appears to vouch for the run; it still prints on that failure (the resources were applied) and the script's error still propagates.