feat(cube-cli): run and follow a dbt sync from the CLI - #11612
Conversation
|
Claude finished @MikeNitsenko's task in 4m 7s —— View job Review: solid work — 1 medium, 3 low. No blockers beyond the
|
| # | Severity | Where | Issue |
|---|---|---|---|
| 1 | Medium | client.rs:338-343 |
res.text() failure now discards the HTTP status, breaking get_optional's 404 detection and the 401/403 messages |
| 2 | Low | deployments.rs:297-305, dbt.rs:50-58 |
--timeout/--poll accepted and silently ignored without --wait |
| 3 | Low | cli.mdx:425-428 |
Prune step runs under bash -e; a failed first delete-branch strands the sync branch |
| 4 | Low (nit) | dbt.rs:365-379 |
--wait can exceed the user's --timeout by up to RESULT_FETCH_TIMEOUT (30s) |
Details are in the inline comments. Only #1 is a behavior regression against master; the rest are UX/docs polish.
Security
Nothing concerning. Notable positives:
- No new credential handling; auth stays on the existing
Clientpath, including the one-shot 401 refresh. shell_quoteon every user-supplied value interpolated into a suggested command is the right posture —git check-ref-formatpermits#,$,;,!, and these messages are read out of CI logs and pasted without rereading.failure_detailcaps response bodies atREASON_LIMITbefore they reach a log, so a gateway HTML page can't bury the progress lines. Nothing matches on that text, so collapsing is free.delete-branchcorrectly defaults--remove-on-upstreamto off, so the connected provider is untouched unless asked.branchNametravels as a query parameter rather than a path segment because names contain slashes — right, and it means no path-injection surface.
Performance
No concerns. Backoff is capped at 60s and overflow-safe (checked_mul → BACKOFF_CAP), MISSING_GRACE/IDLE_GRACE are measured in time rather than polls so --poll 1s can't silently shrink them to six seconds, and progress goes to stderr with label-change dedup so a 15-minute wait produces a handful of lines.
Testing
Coverage is strong for new code — wait.rs, util.rs, client.rs's failure_detail, and dbt.rs's status_label/available_result/wait_json all have focused tests, several written specifically against the failure they replaced. Gaps I noticed, none blocking:
- No test for the
client.rsbody-read-failure path (finding ReferenceError: self is not defined - Issue with whatwg-fetch when using the core lib with Node.js #1) — the regression is invisible to the suite today. wait_for_sync'sMISSING_GRACEandwait_for_build'sIDLE_GRACEexpiry are untested. Both areInstant-based inside a closure, so a test would need injectable time; a cheap partial version would extract the "has the grace expired" decision into a function takingelapsed: Duration.print_resultusesoutput::itemsongeneratedFiles; a non-array value would render as one file. Almost certainly not reachable, but untested.
I could not run cargo test / clippy — cargo isn't in this job's allowed tools, so I reviewed statically. The PR body reports all three passing on 1.90.0.
Documentation
docs-mintlify is the right place, docs.json needs no change (no new pages), anchors resolve (#dbt-sync, #dbt-sync-as-a-ci-test-gate), and --remove-on-upstream / --keep-branch / exit-dev-mode all exist as documented. The <Warning> about $DEV being empty when jq exits 0 without pipefail is a good catch to have written down. Only finding #3 above.
|
Claude finished @MikeNitsenko's task in 1m 31s —— View job Re-review of
|
| # | Prior finding | Fix | Verdict |
|---|---|---|---|
| 1 (Medium) | client.rs — body-read failure discarded the HTTP status |
res.text().await.unwrap_or_default() restored, with a comment naming the three callers that branch on status (404 handling, transient retry, one-shot 401 refresh) |
✅ Correct — this was the only behavior regression against master, and it's gone |
| 2 (Low) | --timeout/--poll silently ignored without --wait |
requires = "wait" on all six args across dbt sync, dbt status, deployments build-status |
✅ Correct, and went one better than I suggested |
| 3 (Low) | Prune step stranded the sync branch under bash -e |
rc=0 / || rc=$? / exit $rc |
✅ Correct — cmd || rc=$? is exempt from -e as the left side of a || list, so both deletes always run and a failure of either still fails the step |
| 4 (Nit) | --wait can exceed --timeout by RESULT_FETCH_TIMEOUT |
dbt sync --timeout help now reads "Give up waiting for the sync after this long; reading its result may take 30s more" |
✅ Took the documentation option, which is the right trade — subtracting elapsed time would have given the final request a budget of zero exactly when the sync ran long |
Notes on the fixes
On #2, the author extended requires to dbt sync too, which I'd explicitly exempted. That's the better call: dbt sync --timeout 1h without --wait is just as inert as the status case, and I was wrong to carve it out. No compatibility concern — --timeout/--poll are new on all three subcommands in this PR, so nothing that parsed on master stops parsing.
The clap requires-with-default_value interaction is the thing that would bite here, and explicit_wait_options_require_wait (util.rs:530) pins both halves: three explicit-without---wait invocations must fail with --wait named in the message, and three bare invocations must still parse despite carrying defaults. Asserting on the error text rather than just is_err() is what makes the first loop meaningful — a required-positional error would otherwise pass it. The test reuses the crate::Cli::try_parse_from pattern already established by only_the_listed_branch_arguments_refuse_an_empty_value, so it exercises the real command tree rather than a hand-built parser.
validate_build_status_args is unaffected — it polices --branch "" with --wait, a different pairing, and clap now handles the timeout/poll case declaratively before that function is reached.
Standing observations (unchanged, no action)
The strengths from my first pass still hold and nothing in this commit weakened them: wait::poll bounding the attempt rather than the gap, the transient-retry whitelist, consecutive-not-cumulative failure counting, shell_quote as a convention rather than an exemption list, and one_line bounding as it goes.
The testing gaps I noted remain, none blocking:
wait_for_sync'sMISSING_GRACEandwait_for_build'sIDLE_GRACEexpiry are untested (bothInstant-based inside closures — would need injectable time, or extraction of the "has the grace expired" decision into a function takingelapsed: Duration).- No test for the
client.rsbody-read-failure path. Finding ReferenceError: self is not defined - Issue with whatwg-fetch when using the core lib with Node.js #1 is fixed, but the fix is still invisible to the suite, so the same regression could return. print_result'soutput::itemson a non-arraygeneratedFiles. Almost certainly unreachable.
Verification caveat
cargo is not in this job's allowed tools, so I reviewed statically and could not run cargo test / clippy / fmt myself. The reasoning above about clap's requires-vs-default_value behavior rests on the new test asserting it, not on my having executed it. The PR body reports all three passing on 1.90.0.
All four of my earlier threads were already resolved before this pass, so nothing needed resolving and no inline comments were posted — there is nothing left outstanding to re-affirm.
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
This PR adds a Cube CLI workflow for running and monitoring dbt syncs in automation. It introduces
cube dbt sync/status/result/cancel, wait and JSON output for CI, and branch cleanup commands while preserving existing optional--branchbehavior.Replaces #11562 with its intended final state on a fresh
masterbase.Summary
cube dbt sync/status/result/cancel, waiting, CI output, and branch cleanup--branchbehavior compatiblebuild-status --wait) and new commandsrefAPI contractValidation
cargo +1.90.0 testcargo +1.90.0 clippy --all-targets -- -D warningscargo +1.90.0 fmt --all --check