feat(cube-cli): add cube dbt generate for CI-side cube generation - #11864
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
github-actions
Bot
added
pr:community
|
Claude finished @MikeNitsenko's task in 2m 41s —— View job Review: 1 blocking, 3 medium, 3 low — the CLI change itself is solid; the commit is carrying 230 unrelated build artifacts.Full review
🔴 Blocking — the commit contains 230 Next.js build artifactsOf the 232 changed files, 230 are
git rm -r --cached docs/.next
printf '.next\n' >> .gitignore # or docs/.gitignore
git commit --amendAdding a 🟠 Medium
🟡 Low
What's goodThe design decisions in the description hold up against the code:
Not verified: I didn't build or run |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Converts a dbt manifest and writes the cube YAML to your working copy instead of committing it to a Cube branch, so a pipeline can commit the result through its own review, CODEOWNERS and branch protection. Cube needs no access to the dbt repository. `--out` is the project ROOT rather than the cube folder: the generated paths are project-relative and are honoured as they come, so the output path saved on the integration keeps deciding where cubes live instead of the CLI re-deriving it. Three things this leans on rather than reinvents. The manifest read and its JSON-object check are `sync --manifest`'s. The wait is `wait_for_sync`. And the file read polls the way the result read does, because a run that has just reported COMPLETED is still closing and can 404 for a moment — which matters more here, since the files are held only briefly and an early 404 treated as the answer would lose the whole run. Two guards run before the wait, so a server doing something other than what was asked is caught in seconds rather than after thirty minutes. A branch name coming back means an older Cube ignored `output` and committed; that is an error naming the branch, because by then it exists in the customer's repository and the review mail has gone out. `resolve_within` refuses absolute paths and any `..` component. These paths come from the API and this is the first place the CLI writes a remote-supplied path to disk, so the check is load-bearing rather than defensive; every path is resolved before any file is written, so a rejected one cannot leave a half-written tree.
4b3dd38 to
ada130f
Compare
|
Claude finished @MikeNitsenko's task in 1m 11s —— View job Re-review: no code change since the last round — 5 open threads still apply (1 high, 2 medium, 2 low)Details
What changedThe Open threads, unchanged
Re-affirmed all 5 prior threads; resolved none, since none is addressed in the current diff. Re-checked this round, still fine
Not run: |
Four findings, all real. The empty-files bail was dead code: the poll only finished on a non-empty payload, so a run that legitimately produced none spun for the full 30s and failed with a generic timeout — the opposite of the message written for it. An answer now ends the poll and only absence is "not yet". A missing `content` was written as a zero-byte file over the committed one, exiting 0. `output::field` yields "" for an absent key, so the one failure this command must not have was the default. Content is now read as a required string and an absent one is a protocol error. `--check` reported a permissions error or a directory in the way as "missing", sending the reader to re-run the write that would fail the same way; only NotFound means "not committed yet" now. Its one-directional limit is real and out of scope here — removing a cube whose dbt model was deleted needs to know which subtree is Cube's to own — so the docs claim is narrowed to what it does rather than left overstated. `--json` printed the start payload, which carries nothing about what was written: strictly less than the human path, for the caller most likely to parse it. It now carries the file list, the output dir and the outcome, following `wait_json`'s precedent. Also trims three doc comments to the sentences that carry the risk.
|
Claude finished @MikeNitsenko's task in 2m 8s —— View job Re-review of
|
| Prior finding | Fix | Verdict |
|---|---|---|
no files bail unreachable; empty run surfaced as a 30s timeout |
Poll now ends on any answer (Some(payload) => Done), only None is "not yet" (dbt.rs:279–282) |
Fixed — bail at :289 is now reachable |
Missing content wrote a zero-byte file over the committed one, exit 0 |
read_generated_files takes content via file.get("content").and_then(Value::as_str) and bail!s on absence (:350–369), with the path named in the message; test a_file_with_no_content_is_refused_rather_than_written_empty |
Fixed, and the as_str form also rejects a non-string content |
Err(_) => "is missing" mislabelled permission / IsADirectory errors |
Only ErrorKind::NotFound is "missing"; other kinds propagate with could not read {path} context (:1027–1036) |
Fixed |
--check's one-directional limit overstated in docs |
cli.mdx now says it does not detect a cube left behind by a deleted dbt model, "which neither --check nor the write path removes" |
Fixed the right way — it states the limit instead of claiming the property |
--json printed the start payload |
generate_json composes syncJobId / outputDir / outcome / fileCount / files (+ differing), wired into all three success exits, with a test |
Fixed on the success paths — see finding 1 for the failure path |
The --out semantics, resolve_within, both pre-wait guards and the resolve-everything-before-writing ordering are unchanged and still right. 3 new tests, 25 total in dbt::.
New this round
🟠 Medium — dbt.rs:1013–1015: --json still prints nothing on the FAILED exit. The precedent this commit invoked (wait_json) emits its document before returning the failure — sync does exactly that at :900–905, and again on the result-read error path. generate returns straight out, so a --json run of a failed generate leaves stdout empty and the syncJobId a pipeline needs to call dbt logs only in a human sentence on stderr. An outcome: "failed" through generate_json covers both exits.
🟡 Low — dbt.rs:327–337: differing is prose, in a different path space than files in the same document. files holds project-relative paths as the server named them; differing holds root-joined display paths with the reason glued on as an English suffix ("/abs/…/orders.yml differs"). It's the one field a gate branches on, so [{"path": …, "status": "differs"}] would let a consumer join the two arrays.
🟡 Low — dbt.rs:289–294: the now-reachable empty-files bail names the wrong cause. With an answer ending the poll, the common way to land there is a run that genuinely produced nothing; expiry now shows up as None and a timeout. "kept only briefly … collect them right after it completes" sends the reader to re-run faster, which does nothing for a project with no cubes to give.
Nit, not posted inline: the three trimmed doc comments came back as new ones — read_generated_files (:342–349, 6 lines) and fetch_generated_files (:253–262, now two paragraphs) both restate in prose what the test beneath them asserts. The load-bearing sentence in the first is one clause: output::field yields "", so a zero-byte write would clobber a committed file. Consistent with the file's prevailing style, so take it or leave it.
Not verified: cargo isn't runnable in this job (command not permitted here), so cargo test dbt:: / clippy / fmt are unrun on my side — findings are read off control flow. The description's unchecked "manual run against the merged server build" box is still the one I'd want ticked: finding 1 and the empty-answer poll change are both server-response-shape behaviour a real run settles immediately.
Resolved no threads — the author had already resolved all 5 from the prior rounds, and each is genuinely addressed in this commit.
Three small ones from review.
A --json run that failed wrote empty stdout: the syncJobId, which is what a
pipeline needs to go read the logs, only reached stderr in an English
sentence. Both failing exits now emit a document with outcome "failed", so
every exit of the command emits exactly one.
`differing` carried prose in a different path space than `files` in the same
document. It now carries {path, status} keyed the way `files` is, which also
made the code smaller: the check loop collects pairs and only the human
branch formats them.
The empty-files message named expiry, which is now the unlikely cause —
expiry arrives as a 404 and a poll timeout. It names the likely one first: a
manifest with no models, or pull options that filtered them all out.
|
Claude finished @MikeNitsenko's task in 1m 28s —— View job Re-review of
|
| Prior finding | Fix | Verdict |
|---|---|---|
--json printed nothing on the FAILED exit |
failed_json() closure emitted on the FAILED branch (:1021–1027) and on the fetch_generated_files error, which is now a match rather than ? (:1029–1038) |
Fixed on both exits named |
differing carried prose, in a different path space than files |
Vec<(String, &str)> of project-relative path + "differs" / "missing"; generate_json emits {"path": …, "status": …}, and only the human branch formats a line (:1048–1074) |
Fixed — and the check loop got smaller, which is the right sign. Test asserts the shape |
| Empty-files bail named expiry, the now-unlikely cause | "generated no cubes. Most likely its manifest had no models to convert, or the deployment's pull options filtered them all out", expiry demoted to a parenthetical (:289–293) |
Fixed — names the cause that actually reaches it, keeps the other |
Unchanged and still right: --out as project root, resolve_within's lexical-only check, both pre-wait guards, and resolving every path before the first write.
New this round
🟡 Low — dbt.rs:1017: the comment claims an invariant the code doesn't hold. "Every exit of this command emits exactly one --json document" — but wait_for_sync(...).await? (:1015), resolve_within in the check loop, and the write loop's create_dir_all / write failures all still write empty stdout, each of them after syncJobId is known. The wait timeout is the one worth weighing: it's the failure a pipeline hits alongside FAILED, and sync --wait has the identical gap — so matching it is defensible, but then the sentence should say "every exit below this point". The partial-write case is the closer call, since it's the analogue of sync's result-read path, which deliberately does emit.
Same hunk, nit: " {} {status}" now prints …/orders.yml missing where it read is missing. Grammar was the only thing the old format string bought and it's gone on that arm only.
Comments
The two new comments in this commit earn their place under the 3-line rule — failed_json's explains why a failing exit needs a document at all (the syncJobId → dbt logs path), and differing's states the keying contract a consumer depends on. The first is the one whose claim needs narrowing, per the finding above.
Not verified: cargo isn't runnable in this job (command not permitted), so cargo test dbt:: / clippy / fmt are unrun on my side — the PR body reports 25 tests + 2 new assertions clean on +1.90.0. The description's unchecked "manual run against a deployment on the merged server build" is the last thing I'd want ticked: the --json document shape and the empty-answer poll rule are both settled in seconds by one real run.
Resolved no threads — the author had already resolved all 8 from the prior rounds, and each is genuinely addressed.
The comment claimed every exit emits a document; several still do not — the wait above it, the path checks, and the write loop. A comment stating an invariant the code lacks is worse than no comment, since the next edit trusts it. Narrowed to the two exits it does cover, and it now says why the others are left: the wait matches `sync --wait`, and a write failure leaves the tree PARTIALLY written, where `outcome: "failed"` would be a lie. Telling that apart needs an outcome this command does not have, which is not worth inventing on speculation. Also restores the grammar the human line lost when `status` became a token: `differs` / `is missing` on the printed line, plain tokens in the JSON.
|
Claude finished @MikeNitsenko's task in 1m 25s —— View job Re-review of
|
| Prior finding | Fix | Verdict |
|---|---|---|
Comment claimed an invariant ("every exit emits exactly one --json document") the code doesn't hold |
Narrowed to "the two failures BELOW", with a second paragraph naming the two exits deliberately left out and why (:1017–1024) |
Fixed the right way — the why for the write loop is the part that matters: outcome: "failed" over a partially-written tree would be a lie, and the document has no outcome for "some files landed" |
" {} {status}" printed …/orders.yml missing |
phrase maps missing → is missing for the human line only; differing[].status stays the bare token (:1075–1086) |
Fixed |
On leaving the write-loop failures silent under --json: I agree with the reasoning in your reply. failed_json() there would assert files: [] while files are on disk, and a caller acting on it would skip a git add it needs — a partial outcome carrying what was written is the honest shape, and inventing it without a caller asking for it is the worse trade. The comment now records that, which is what keeps the next edit from "fixing" it.
Unchanged and still right: --out as project root, resolve_within's lexical-only check, both pre-wait guards, resolving every path before the first write, and read_generated_files treating a missing content as a protocol error.
New this round
🟡 Low — dbt.rs:1078–1086: the new three-line comment restates the three lines under it. let phrase = if *status == "missing" { "is missing" } else { "differs" } already says that the printed line reads as a sentence while the stored token stays a token — there's no decision here a later edit could undo without noticing. Same hunk, a match would also close the else arm, which today maps any status token added later to "differs".
Comments, generally
Two of the three comments touched this round earn their place: the failed_json block and generate_json's differing keying both state contracts a reader can't recover from the code. The failed_json one runs eight lines against the repo's three-line guideline — but the second paragraph is precisely what the last round asked for, and I can't state "a write failure leaves the tree partially written, so failed would be a lie" in materially less. Leaving it.
Not verified: cargo is present in this job but running it isn't permitted, so cargo test dbt:: / clippy / fmt are unrun on my side — findings read off control flow. The PR body reports them clean on +1.90.0. The description's unchecked "manual run against a deployment on the merged server build" remains the one thing I'd want ticked before merge: the --json document shape and the "an answer, empty or not, ends the poll" rule are both settled in seconds by one real run.
Resolved no threads — the author had already resolved all 9 from the prior rounds, and each is genuinely addressed.
The comment said what the three lines under it already said, and recorded nothing a later edit could silently undo. The `if/else` it explained also mapped any status token added later to "differs"; a match with `other => other` closes that and is shorter than what it replaces.
|
Claude finished @MikeNitsenko's task in 39s —— View job Re-review of
|
| Prior finding | Fix | Verdict |
|---|---|---|
Three-line comment restating the phrase mapping under it; else arm mapped any future status token to "differs" |
Comment gone, if/else → match *status { "missing" => "is missing", other => other } (dbt.rs:1077–1080) |
Fixed exactly as suggested — and a net −4 lines, which is the right shape for a comment finding |
The other => other arm is the part that matters beyond the comment: a status token added later now prints itself rather than silently claiming differs, and differing[].status in the JSON document stays the bare token either way.
Re-checked this round, unchanged and still right
--outas the project root, withresolve_withindoing a lexical-only check (absolute rejected, every non-Normal/CurDircomponent rejected, empty rejected) — and every path resolved before the firststd::fs::write, so a rejected path can't leave a half-written tree.- Both pre-wait guards (
ensure_nothing_was_committed,verify_manifest_source), catching a server doing something other than what was asked in seconds rather than after the 30-minute wait. read_generated_filestreating an absentcontentas a protocol error naming the path, rather than writing a zero-byte file over a committed one.- The
--jsoncontract and the comment that bounds it: two failing exits below the wait emitoutcome: "failed"; the wait and the write loop deliberately don't, with the reason ("failed"over a partially-written tree would be a lie) recorded where the next edit will read it. fetch_generated_files: an answer, empty or not, ends the poll; only absence is "not yet" — and the empty-files bail names the cause that actually reaches it.- 25 tests in
dbt::, including the three that cover this feature's load-bearing behaviour (generate payload asks forfilesand never a branch; a returned branch name is refused; hostile paths refused).
Not verified: cargo is present in this job but running it isn't permitted, so cargo test dbt:: / clippy / fmt are unrun on my side — the PR body reports them clean on +1.90.0, and this commit touches three lines inside one else branch. The description's unchecked "manual run against a deployment on the merged server build" is the one thing I'd still want ticked before merge: the --json document shape and the "an answer ends the poll" rule are both settled in seconds by one real run.
Resolved no threads — all 10 from prior rounds were already resolved by the author, and each is genuinely addressed in the current diff.
The integration page taught one topology — Cube converts and commits to a review branch — and said nothing about the other, which is the whole reason `cube dbt generate` exists. A customer whose data model lives in their own Git repository had no way to find it. Adds the choice where automated syncs are introduced, framed by the question that actually decides it (who commits the cubes), and a section with a complete CI job. The job takes the manifest as an artifact rather than from a dbt project checked out beside it, since that is the shape people will actually have: no dbt repository anywhere near the runner. Also covers `--check` as the PR gate, including what it does not catch — a cube orphaned by a deleted dbt model — so the page does not promise more than the command does.
|
Claude finished @MikeNitsenko's task in 1m 58s —— View job Re-review of
|
Three review findings, all real. The job was billed as complete and could not run: `ubuntu-latest` has no `cube` binary, and `GITHUB_TOKEN` is read-only by default, so the step that generates and the step that pushes both failed. Adds the install step and `permissions: contents: write`. `git add model/cubes/dbt` hard-coded the default Output path two paragraphs after saying that setting decides where cubes land. On a deployment using a different one it stages nothing, `git diff --cached --quiet` succeeds, and the job goes green over a working copy that drifted — a false green on the only step that matters. `git add -A` instead; the CLI writes nowhere else. The sample triggers on push and commits, but the sentence under it argued about pull requests. Moved the notification point down to the `--check` variant, which is the one that runs per PR and commits nothing.
|
Claude finished @MikeNitsenko's task in 1m 29s —— View job Re-review of
|
| Prior finding | Fix | Verdict |
|---|---|---|
The "complete job" couldn't run: no cube binary, and git push would 403 |
permissions: contents: write on the job (:524–526) and an Install the Cube CLI step (:534–535) |
Fixed. The install line matches reference/cli.mdx#installation verbatim and install-cli.sh exists at that path in master; it falls back to $HOME/.local/bin when /usr/local/bin isn't writable, and both are on a hosted runner's PATH |
git add model/cubes/dbt hard-coded the default Output path — a false green on a deployment configured with a different one |
git add -A (:551) |
Fixed |
Sample was on: [push] while the prose under it reasoned about pull requests |
Notification point moved down to sit with --check (:582–584), and the paragraph under the sample narrowed to "no branch is created in your Cube data model" |
Fixed the right way — the every-PR frequency it invokes is true of --check and only of --check |
Also checked and fine: the commit-back-on-push job doesn't loop, since a push made with GITHUB_TOKEN doesn't trigger another workflow run; checkout@v4 on a push event leaves a real branch, so the bare git push works; git diff --cached --quiet && echo … && exit 0 is still safe under set -e.
New this round
🟡 Low — dbt.mdx:547–551: four lines of rationale inside a snippet the reader copies into their own repo. What it argues against is the review round that produced the line rather than something a later edit of their workflow could reintroduce, and it lands in their file. One clause carries it: the CLI writes only under the Output path, so -A avoids hard-coding it. Same comment, worth noting: -A also stages anything else a reader's job leaves untracked, which the hard-coded path didn't — still the right default for a sample that can't know the output path, but a shorter comment doesn't oversell it.
Unchanged and still right
No Rust in this commit, so everything verified through 11060b5 stands: --out as project root, resolve_within's lexical-only check, both pre-wait guards, resolve-everything-before-writing, read_generated_files treating an absent content as a protocol error, and the bounded --json contract with the reason for its two deliberate gaps recorded where the next edit will read it.
On the page itself: the framing by who commits the generated cubes remains the best thing here, the --check limitation is stated rather than hedged and matches the code, and both anchors (#generate-cubes-without-committing-them, #configure-pull-settings) resolve.
Not verified: no code changed, so no build or test was called for. The PR description's "manual run against a deployment on the merged server build" box is still the one outstanding item from the code rounds.
Resolved no threads — all 13 from prior rounds were already resolved by the author, and this commit doesn't re-open any.
The four lines argued against the review round that produced them, not against anything a later edit of the reader's own workflow could reintroduce — and this snippet is copied verbatim into their file. One clause carries what they need to know.
|
Claude finished @MikeNitsenko's task in 27s —— View job Re-review of
|
| Prior finding | Fix | Verdict |
|---|---|---|
| Four lines of rationale inside a snippet the reader copies verbatim into their own repo, arguing against the review round that produced it | Two lines: "The CLI writes only under the deployment's Output path, so -A avoids hard-coding it here." (dbt.mdx:547–548), net −2 |
Fixed as suggested. It keeps the one clause a reader needs to decide whether -A is right for their job, and drops the false-green narration — which was our history, not theirs |
Nothing else changed: the commit is 2 insertions, 4 deletions, in one snippet comment.
Re-checked this round, unchanged and still right
The CI sample still runs as written — permissions: contents: write, the install-cli.sh step matching reference/cli.mdx#installation, download-artifact supplying the manifest with no dbt project on the runner, and git diff --cached --quiet && echo … && exit 0 safe under Actions' bash -e (non-final operand of an && list). The --check paragraph still states its one-directional limit rather than claiming the stronger property, and both anchors resolve.
No Rust in this commit, so everything verified through 11060b5 stands: --out as the project root, resolve_within's lexical-only check with every path resolved before the first write, both pre-wait guards, read_generated_files treating an absent content as a protocol error, and the bounded --json contract with the reason for its two deliberate gaps recorded where the next edit will read it.
Not verified: no code changed, so no build or test was called for. The PR description's "manual run against a deployment on the merged server build" box is the one outstanding item from the code rounds.
Resolved no threads — all 14 from prior rounds were already resolved by the author, and this commit doesn't re-open any.
…eference The command table lists one row per top-level command, with each command's subcommands packed into its Description cell — where they were listed at all. A reader scanning the Command column for a command they had not seen before cannot find `enable-branch` there, even though it was added to the `data-model` cell in cube-js#11433. Keep that table as a flat index of top-level commands, and add subcommand tables below it grouped by domain: authentication, deployment, GitHub, data model, dbt, workspace content, users and access control, administration, embedding, and agents/app. Each hierarchical row in the index links to its group, and the narrative sections link into the same tables rather than repeating them. Every subcommand is taken from `cube <command> --help` (CLI 1.7.39), so the tables match what the binary reports, including third-level ones (`notifications recipients`, `integrations tokens`, `embed tenant`, `attributes values`, `scim users`/`groups`) that the page did not mention at all. This also restores `exit-dev-mode` and `file-hashes`, both used by the Git workflow section but missing or unexplained before. The dbt table covers `dbt generate` and the `--manifest` sync option from cube-js#11845 and cube-js#11864, and links to the section that describes them. The `enable-branch` prose gets its own heading so the tables have an anchor to link to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
2 participants
Summary
The CLI half of CUB-4489.
cube dbt generateconverts a dbt manifest and writes the cube YAML to your working copy — committing nothing, creating no branch — so a pipeline commits the result through its own review, CODEOWNERS and branch protection. Cube needs no access to the dbt repository.dbt parse cube dbt generate DEPLOYMENT_ID --manifest target/manifest.json --out .The server side (
output: "files"plusGET /dbt-sync/{id}/generated-files) merged in cubedevinc/cubejs-enterprise#14922.Decisions worth a look
--outis the project ROOT, not the cube folder. Generated paths are project-relative (model/cubes/dbt/…) and are written beneath it as they come, so the output path saved on the integration keeps deciding where cubes live. The alternative — stripping the prefix — would re-derive a server-side decision on the client.outputand committed anyway; that's an error naming the branch, because by then it exists in the customer's repo and the review mail has gone out. This mirrors the existingensure_manifest_source.COMPLETEDis still closing and can 404 for a moment — and it matters more here, because these files are held only briefly, so treating an early 404 as the answer would lose the entire output.resolve_withinrefuses absolute paths and... These paths come from the API, and this is the first place the CLI writes a remote-supplied path to disk, so it's load-bearing rather than defensive. Every path is resolved before any file is written, so a rejected one can't leave a half-written tree.Reuse
The manifest read and its JSON-object check,
wait_for_sync, andverify_manifest_sourceare allsync --manifest's, unchanged.Docs
Both halves of CUB-4489's docs condition:
reference/cli.mdx— thegeneratecommand,--outsemantics,--check, and what--checkdoes not catch.docs/integrations/dbt.mdx— the page taught only "Cube converts and commits to a review branch" and said nothing about the topology this feature exists for. It now frames the choice by the question that decides it (who commits the cubes) and carries a complete CI job that takes the manifest as an artifact, since that is the realistic shape: no dbt repository anywhere near the runner.Test plan
cargo test dbt::— 22 passing, including three new: the generate payload asks forfilesand never a branch; a returned branch name is refused with the branch named; hostile paths (../,model/../../, absolute, empty) are refusedcargo clippy --all-targetsclean,cargo fmt --checkcleanstaging-mikenitsenko-2on the merged server build:Verified on the live tenant: files land under
--outat the server-suppliedpaths with real content (1382 / 1553 / 1491 / 1255 bytes);
--checkpassesagainst what was just written;
--checkafter tampering with one file anddeleting another reports
differs/is missingand exits 1;--jsononthat path emits
outcome: "differing"with{path, status}entries; and thesync history shows
branchNameempty for all five runs, so nothing wascommitted and no branch was created.
One thing the run turned up that is NOT from this PR: while ai-engineer was
restarting, a plain committing sync returned 500 after
launchDbtSynchadalready created the branch row, leaving an orphaned
dbt-sync/*branch with nohistory row behind it. The server guards this ordering against a missing repo
config but not against an ai-engineer outage. Filing separately.
Note for reviewers: local
stablehere is 1.86 and a dependency now needs 1.88, so the above ran on+1.90.0.