feat: match a customer-configured manifest filename - #2265
pankajastro wants to merge 9 commits into
Conversation
Coverage Report for CI Build 36707016055Coverage increased (+0.01%) to 44.86%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
|
|
||
| // ManifestName, when set, replaces manifest.json as the filename | ||
| // findManifests matches - for a manifest under a different name, or to | ||
| // pick one out of several valid manifests in the same directory. |
There was a problem hiding this comment.
The plugin finds both artifacts by directory, not by manifest: slim_manifest_path() is manifest_path.parent / ".astro" / "manifest.slim.json" and the hash comes from manifest_path.parent / ".astro" / "dbt_metadata.json" (deploy_metadata.py). So in the BOSS-697 layout, any DbtDag whose manifest_path is manifests_per_schedule/manifest_global_daily_schedule.json would now load the slim copy of manifest_full.json (it was written at deploy time, so the mtime check in graph.py passes) and key its caches on manifest_full.json's hash. I ran Run on that layout and the only .astro/ in the directory is manifest_full's.
The same hazard existed before for a sibling next to manifest.json, but picking one manifest out of several in a directory is the use case this PR is for. Should the sidecar record the source manifest's filename, with the plugin checking it against manifest_path before serving either artifact? Until the plugin checks, skipping the slim and sidecar when the directory holds another dbt manifest would at least avoid serving the wrong graph.
There was a problem hiding this comment.
Ideally a dbt project has exactly one manifest, named manifest.json. We found a customer whose manifest uses a custom name - that's the original ask here. I've handled the case of multiple dbt projects each using a different custom name (rather than assuming every project in a deploy shares the same one), via a per-directory JSON map instead of one global override.
For the specific hazard you raised - two valid manifests sharing one directory - added hasSiblingDbtManifest: before writing anything, checks whether another valid dbt manifest shares the directory, and skips the whole unit if so, rather than guessing. So for manifests_per_schedule/ specifically, the override now safely stamps nothing there instead of risking wrong data - it still helps a directory with exactly one custom-named manifest, just not this two-live-manifests case.
| // manifestNameOverride returns "" when unset, leaving manifest.json as the | ||
| // only name discovery matches. | ||
| func manifestNameOverride() string { | ||
| return strings.TrimSpace(os.Getenv(manifestNameEnvVar)) |
There was a problem hiding this comment.
findManifests compares against d.Name(), so a value like target/manifest_full.json never matches. Because the override replaces manifest.json, that deploy then stamps no manifests at all, and the only trace is the debug report (Run with that value returns 0 results and a nil error). Could we reject a value where filepath.Base(v) != v with an error that names ASTRO_COSMOS_BOOST_MANIFEST_NAME?
There was a problem hiding this comment.
Fixed - manifestNameOverrides now rejects any value where filepath.Base(v) != v with an error naming ASTRO_COSMOS_BOOST_MANIFEST_NAME, instead of silently matching nothing. (The env var itself also changed shape since this comment - it's now a JSON directory->filename map, not a single name, so a customer with two projects using different conventions can express both in one deploy.)
| var filtered *FilteredManifest | ||
| if opts.SlimManifest { | ||
| if doc, _, isDbt, readErr := readManifestDoc(filepath.Join(dir, manifestFile)); readErr == nil && isDbt { | ||
| name := manifestFile |
There was a problem hiding this comment.
nit: this repeats the default from findManifests. Resolving it once at the top of Run (if opts.ManifestName == "" { opts.ManifestName = manifestFile }) would let both use opts.ManifestName directly and drop the empty check in findManifests.
There was a problem hiding this comment.
Done - factored into effectiveManifestName(overrides, root, dir), shared by findManifests and processProject; neither has its own default-resolution branch anymore.
ASTRO_COSMOS_BOOST_MANIFEST_NAME, when set, replaces manifest.json as the only filename Cosmos Boost pre-deploy discovery matches. A customer whose manifest isn't named manifest.json (and may keep other, differently-purposed manifests in the same directory) had nothing discovered at all; setting this env var to their manifest's actual name picks up exactly that file, wherever it sits in the tree, and leaves every other file untouched. Unset, behavior is unchanged. Co-Authored-By: Claude <noreply@anthropic.com>
ASTRO_COSMOS_BOOST_MANIFEST_NAME now holds a JSON object mapping a directory (relative to the deployed path, "." for its root) to the filename discovery should match there, instead of one name applied to the whole deploy. A single global name couldn't represent different projects using different manifest naming conventions in one deploy; an unlisted directory still falls back to manifest.json. Also rejects malformed JSON and any value that isn't a bare filename (a path there would silently match nothing in findManifests' basename comparison), failing the deploy step instead of stamping nothing silently. Co-Authored-By: Claude <noreply@anthropic.com>
The plugin resolves both the hash sidecar and the slim manifest by directory alone, with no check of which manifest produced them. If a directory legitimately holds more than one valid dbt manifest (e.g. per-schedule manifests read by different DAGs), stamping it for one manifest would risk serving that manifest's slim copy and cache hash to a DAG that actually points at the other. hasSiblingDbtManifest checks, before writing anything, whether another *.json file in the same directory also parses as a valid dbt manifest. When it does, processManifest skips both artifacts for that unit (like a non-dbt manifest, but with a Warning naming the reason) and processProject skips only the slim attachment, since the project's own tree-hash sidecar isn't manifest-specific and stays safe to write. Co-Authored-By: Claude <noreply@anthropic.com>
- effectiveManifestName: drop the len(overrides)==0 fast path - a nil
map lookup already returns ("", false) safely, so it was dead code.
- manifestNameOverrides: filepath.Base("") == "." != "", so the
explicit name == "" check was redundant with the bare-filename check.
- Inline joinWarnings (used at exactly one call site) instead of a
standalone helper.
- Drop TestRunManifestNameOverrideInProjectRootIsSlimmedNotDiscovered:
fully subsumed by TestRunManifestNameOverridePerDirectory's dbt1 case.
- Replace two PreDeploy-level rejection tests (each spinning up a
tempdir to hit an error that happens before any filesystem access)
with one direct TestManifestNameOverrides covering unset/valid/
invalid-JSON/path-valued cases against the parser itself.
- Shrink processManifest's doc comment to a pointer at
hasSiblingDbtManifest instead of restating its rationale.
Co-Authored-By: Claude <noreply@anthropic.com>
No behavior change. Doc comments added across this PR's earlier commits restated what the code already shows; cut each to the non-obvious "why" (or the keying/format convention where that isn't otherwise visible), dropping the rest. Co-Authored-By: Claude <noreply@anthropic.com>
isDbtManifest checked only for metadata.dbt_schema_version's presence, which every dbt artifact carries (run_results.json, catalog.json, sources.json, semantic_manifest.json - not just manifest.json), each under its own schema URL. On main this was harmless: it only ever ran on a file already found by exact filename match. hasSiblingDbtManifest breaks that assumption by scanning every *.json in a directory by content - so a plain "dbt build" (which always writes run_results.json next to manifest.json in target/) would flag the directory ambiguous and silently stop stamping manifest.json for most real projects. Tightened to require the schema URL contain "/manifest/", which every dbt manifest schema does and no other dbt artifact's does (confirmed against schemas.getdbt.com). Also fixed two related error-handling gaps found in the same pass: processProject aborted the whole unit (skipping its always-safe tree-hash sidecar) when the sibling check itself failed to read the directory, instead of just skipping the slim attachment the way an actually-ambiguous directory does; and processManifest promoted that same read failure to a hard Err instead of the same fail-safe Skipped+Warning an ambiguous directory gets. Co-Authored-By: Claude <noreply@anthropic.com>
dir always descends from root (it only ever comes from walking root), so filepath.Rel(root, dir) can't realistically fail here - and even if it did, Rel returns "" on error, which already can't match a real override key (the documented convention is "." for root). The explicit early-return added nothing a plain fallthrough doesn't already do correctly. Co-Authored-By: Claude <noreply@anthropic.com>
The override key is a path relative to the whole deploy root, not to the project's own directory - worth pinning since a dbt project living under dags/ (dags/dbt/<name>/) is a common Astro layout, and it would be easy to assume the key is just the project's own folder name. Co-Authored-By: Claude <noreply@anthropic.com>
34347fa to
c6117a6
Compare
Rewritten from scratch, not incrementally: discovery no longer relies on any customer-configured filename. A file is treated as a manifest when its name contains "manifest" (case-insensitive) and its content validates as one (isDbtManifest) - covering manifest.json, manifest_full.json, manifest_by_schedule.json, etc. with nothing to configure. ASTRO_COSMOS_BOOST_MANIFEST_NAME, Options.ManifestNames, effectiveManifestName, and hasSiblingDbtManifest are all removed. Every manifest found now gets its own slim companion, named after itself (slimNameFor: "manifest_full.json" -> "manifest_full.slim.json", "manifest.json" -> "manifest.slim.json" as before) - so N manifests in one directory produce N slim files, never a single contested one. A project root with multiple manifests slims each in turn. Cleanup matches slim files by their .slim.json suffix instead of one fixed name, so it can find and remove any of them; ownership is still checked per file via its own _generated_by marker. .astro/dbt_metadata.json is untouched - same fixed name, same schema, same writer - so a directory with multiple manifests still gets one sidecar, written by whichever unit's processing finishes last. Co-Authored-By: Claude <noreply@anthropic.com>
bd6e318 to
a82ea27
Compare
|
@kaxil heads up - the approach changed significantly since my earlier replies on this thread, so those are superseded. Instead of a customer-configured env var (name override, then a JSON directory→filename map) plus an ambiguity guard that skipped stamping when a directory held more than one valid manifest, this now discovers manifests automatically: a file is treated as a manifest when its name contains "manifest" (case-insensitive) and its content validates as one - no configuration needed. The bigger change: every manifest found now gets its own slim companion, named after itself ( PR description is updated to match. Would appreciate another look when you have time. |
This branch has not been deployed
2 participants
Summary
Approach changed since earlier commits/review — replaced the customer-configured env var with automatic discovery. No configuration needed.
Discovery previously matched only files literally named
manifest.json. Now a file is treated as a manifest when its name contains "manifest" (case-insensitive) and its content validates as one - coveringmanifest.json,manifest_full.json,manifest_by_schedule.json, etc. automatically.Every manifest found gets its own slim companion, named after itself:
So a directory with multiple manifests (e.g. per-schedule manifests, each read by a different DAG) gets one slim file per manifest instead of one contested file - no picking a winner, no collision. Three dbt projects with two manifests each produce six slim files, one per manifest.
.astro/dbt_metadata.json(the hash sidecar) is unchanged: same fixed name, same schema, same writer. A directory with multiple manifests still gets one sidecar, written by whichever manifest's processing finishes last.Cleanupnow matches a slim file by its.slim.jsonsuffix instead of one fixed name, so it can find and remove any of them; ownership is still checked per file via its own_generated_bymarker before deleting.Testing
go build/go vet/gofmtclean; fullpkg/cosmosboost/...suite passing (golang:1.26 container - no local Go toolchain).TestRunDiscoversCustomNamedManifest,TestRunSlimsEveryManifestInADirectory,TestRunSlimsEveryManifestInProjectRoot,TestRunHandlesMultipleProjectsWithDifferentManifestNames,TestRunDoesNotFlagOtherDbtArtifactsAsManifests,TestIsManifestCandidateName,TestSlimNameFor,TestCleanupRemovesCustomNamedSlimManifest,TestCleanupKeepsForeignSlimJSONSuffixedFile.🤖 Generated with Claude Code