Refactor validate_direct_only_resources and approval log loops#5215
Merged
Conversation
Drop the local directOnlyResource struct (whose field names misnamed SingularTitle/SingularName as pluralName/singularName) and the per-type getResources closures. Iterate AllResources() and key off PluralName instead, reading SingularTitle/SingularName from the canonical ResourceDescription. Add a unit test covering direct/terraform engines and each direct-only resource type. Co-authored-by: Isaac
Replace the eight (deploy) and seven (destroy) near-identical
'if len(...) != 0 { LogString(message); for _ { Log(action) } }'
blocks with a table-driven helper, logApprovalGroups, that takes a
slice of {group, message, skipChildren, trailingGap} entries.
deploy collapses the eight-clause len()==0 early-return to total==0
returned from the helper. The schema child-resource skip and the
destroy trailing blank lines are preserved via the per-group flags.
The outer "all deletes" preamble in destroy stays as-is because it
is structurally different (it skips children and has no trailing
banner per group).
Co-authored-by: Isaac
janniklasrose
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
janniklasrose
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
denik
approved these changes
May 8, 2026
| }, | ||
| // directOnlyResourceTypes lists resources only supported in direct deployment mode. | ||
| // Keys are PluralName values from resources.ResourceDescription. | ||
| var directOnlyResourceTypes = map[string]bool{ |
Contributor
There was a problem hiding this comment.
Could we make use of these two maps to automatically figure this out?
bundle/deploy/terraform/pkg.go:var GroupToTerraformName = map[string]string{
bundle/deploy/terraform/pkg.go- // 2 level groups: resources.GROUP
bundle/deploy/terraform/pkg.go- "jobs": "databricks_job",
bundle/deploy/terraform/pkg.go- "pipelines": "databricks_pipeline",
bundle/deploy/terraform/pkg.go- "models": "databricks_mlflow_model",
bundle/deploy/terraform/pkg.go- "experiments": "databricks_mlflow_experiment",
bundle/deploy/terraform/pkg.go:var GroupToTerraformName = map[string]string{
bundle/direct/dresources/all.go:var SupportedResources = map[string]any{
bundle/direct/dresources/all.go- "jobs": (*ResourceJob)(nil),
bundle/direct/dresources/all.go- "pipelines": (*ResourcePipeline)(nil),
bundle/direct/dresources/all.go- "experiments": (*ResourceExperiment)(nil),
bundle/direct/dresources/all.go- "catalogs": (*ResourceCatalog)(nil),
bundle/direct/dresources/all.go- "schemas": (*ResourceSchema)(nil),
Contributor
Author
There was a problem hiding this comment.
Done in 6e6940d — isDirectOnly(name) now checks dresources.SupportedResources minus terraform.GroupToTerraformName.
| {group: "database_instances", message: deleteDatabaseInstanceMessage, trailingGap: true}, | ||
| {group: "synced_database_tables", message: deleteSyncedDatabaseTableMessage, trailingGap: true}, | ||
| {group: "postgres_projects", message: deletePostgresProjectMessage, trailingGap: true}, | ||
| {group: "postgres_branches", message: deletePostgresBranchMessage, trailingGap: true}, |
Contributor
There was a problem hiding this comment.
trailingGap is always true, is that intentional?
Contributor
Author
There was a problem hiding this comment.
kinda, because approvalGroup is shared between deploy and destroy. But on second thought it's probably better to omit per-resource boolean and pass it as a single parameter to logApprovalGroups
Contributor
Author
There was a problem hiding this comment.
Done in c3bef30 — trailingNewline is now a parameter to logApprovalGroups (also renamed from trailingGap since "newline" describes what cmdio.LogString(ctx, "") actually emits).
Replace the hardcoded directOnlyResourceTypes set with a check against dresources.SupportedResources and terraform.GroupToTerraformName: a resource type is direct-only if it appears in the former but not the latter. Removes the third source of truth for which resource types exist where. Co-authored-by: Isaac
Drop the per-group trailingGap field on approvalGroup (it was always true for destroy and never set for deploy) and pass a single bool parameter to logApprovalGroups instead. Also rename the concept from "gap" to "newline" since that's what cmdio.LogString(ctx, "") prints. Co-authored-by: Isaac
janniklasrose
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
janniklasrose
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
denik
pushed a commit
that referenced
this pull request
May 20, 2026
## Changes
Two independent refactors in `bundle/`:
1. **`bundle/config/mutator/validate_direct_only_resources.go`** — drop
the local `directOnlyResource` struct (whose `pluralName`/`singularName`
fields actually held `SingularTitle`/`SingularName` values) and the
per-type `getResources` closures. Iterate
`b.Config.Resources.AllResources()` and key off `PluralName` via a small
`map[string]bool` instead, reading `SingularTitle`/`SingularName` from
the canonical `ResourceDescription`. Adds a unit test covering
direct/terraform engines × the three direct-only resource types.
2. **`bundle/phases/{deploy,destroy}.go`** — collapse the eight (deploy)
and seven (destroy) near-identical `if len(xActions) != 0 {
LogString(message); for _ { Log(action) } }` blocks into a table-driven
helper `logApprovalGroups` in a new `bundle/phases/approval.go`. The
deploy version also replaces the eight-clause `len(...) == 0 &&`
early-return with a single `total == 0` check returned from the helper.
The schema child-resource skip (deploy only) and trailing blank lines
(destroy only) are preserved via per-group `skipChildren`/`trailingGap`
flags. The outer "all deletes" preamble in destroy stays as-is — it's
structurally different.
Net diff: −215 source LOC, +110 test LOC.
## Why
Both files had grown into mechanical repetition.
`validate_direct_only_resources.go` re-declared resource metadata that
already lives on each resource's `ResourceDescription()` and named the
fields incorrectly. The approval functions repeated the same
eight-/seven-times pattern inline, with an opaque eight-clause boolean
expression for the early-return.
There is one minor user-visible wording change: for `external_locations`
and `vector_search_endpoints`, the Detail message now reads `... use
external_location resources.` / `... use vector_search_endpoint
resources.` (snake_case, from `SingularName`) instead of `... use
external location resources.` / `... use vector search endpoint
resources.` (the old struct's hand-written field with spaces). The
catalog message is byte-identical. There are no acceptance tests
covering the other two messages.
## Tests
- New unit test `validate_direct_only_resources_test.go` (table-driven
over the three direct-only types).
- Existing acceptance test
`bundle/validate/catalog_requires_direct_mode` passes byte-identically.
- `bundle/destroy/...`, `bundle/deploy/...` (excluding the pre-existing
`spark-jar-task` Java env failure that also fails on `main`),
`bundle/resources/grants/schemas/...`, `bundle/config-remote-sync/...`,
and `bundle/user_agent/simple` all pass byte-identically.
- `./task fmt`, `./task checks`, `./task lint` clean.
_This PR was written by Claude Code._
TanishqDatabricks
pushed a commit
to TanishqDatabricks/cli
that referenced
this pull request
May 22, 2026
…ricks#5215) ## Changes Two independent refactors in `bundle/`: 1. **`bundle/config/mutator/validate_direct_only_resources.go`** — drop the local `directOnlyResource` struct (whose `pluralName`/`singularName` fields actually held `SingularTitle`/`SingularName` values) and the per-type `getResources` closures. Iterate `b.Config.Resources.AllResources()` and key off `PluralName` via a small `map[string]bool` instead, reading `SingularTitle`/`SingularName` from the canonical `ResourceDescription`. Adds a unit test covering direct/terraform engines × the three direct-only resource types. 2. **`bundle/phases/{deploy,destroy}.go`** — collapse the eight (deploy) and seven (destroy) near-identical `if len(xActions) != 0 { LogString(message); for _ { Log(action) } }` blocks into a table-driven helper `logApprovalGroups` in a new `bundle/phases/approval.go`. The deploy version also replaces the eight-clause `len(...) == 0 &&` early-return with a single `total == 0` check returned from the helper. The schema child-resource skip (deploy only) and trailing blank lines (destroy only) are preserved via per-group `skipChildren`/`trailingGap` flags. The outer "all deletes" preamble in destroy stays as-is — it's structurally different. Net diff: −215 source LOC, +110 test LOC. ## Why Both files had grown into mechanical repetition. `validate_direct_only_resources.go` re-declared resource metadata that already lives on each resource's `ResourceDescription()` and named the fields incorrectly. The approval functions repeated the same eight-/seven-times pattern inline, with an opaque eight-clause boolean expression for the early-return. There is one minor user-visible wording change: for `external_locations` and `vector_search_endpoints`, the Detail message now reads `... use external_location resources.` / `... use vector_search_endpoint resources.` (snake_case, from `SingularName`) instead of `... use external location resources.` / `... use vector search endpoint resources.` (the old struct's hand-written field with spaces). The catalog message is byte-identical. There are no acceptance tests covering the other two messages. ## Tests - New unit test `validate_direct_only_resources_test.go` (table-driven over the three direct-only types). - Existing acceptance test `bundle/validate/catalog_requires_direct_mode` passes byte-identically. - `bundle/destroy/...`, `bundle/deploy/...` (excluding the pre-existing `spark-jar-task` Java env failure that also fails on `main`), `bundle/resources/grants/schemas/...`, `bundle/config-remote-sync/...`, and `bundle/user_agent/simple` all pass byte-identically. - `./task fmt`, `./task checks`, `./task lint` clean. _This PR was written by Claude Code._
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
2 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.
Changes
Two independent refactors in
bundle/:bundle/config/mutator/validate_direct_only_resources.go— drop the localdirectOnlyResourcestruct (whosepluralName/singularNamefields actually heldSingularTitle/SingularNamevalues) and the per-typegetResourcesclosures. Iterateb.Config.Resources.AllResources()and key offPluralNamevia a smallmap[string]boolinstead, readingSingularTitle/SingularNamefrom the canonicalResourceDescription. Adds a unit test covering direct/terraform engines × the three direct-only resource types.bundle/phases/{deploy,destroy}.go— collapse the eight (deploy) and seven (destroy) near-identicalif len(xActions) != 0 { LogString(message); for _ { Log(action) } }blocks into a table-driven helperlogApprovalGroupsin a newbundle/phases/approval.go. The deploy version also replaces the eight-clauselen(...) == 0 &&early-return with a singletotal == 0check returned from the helper. The schema child-resource skip (deploy only) and trailing blank lines (destroy only) are preserved via per-groupskipChildren/trailingGapflags. The outer "all deletes" preamble in destroy stays as-is — it's structurally different.Net diff: −215 source LOC, +110 test LOC.
Why
Both files had grown into mechanical repetition.
validate_direct_only_resources.gore-declared resource metadata that already lives on each resource'sResourceDescription()and named the fields incorrectly. The approval functions repeated the same eight-/seven-times pattern inline, with an opaque eight-clause boolean expression for the early-return.There is one minor user-visible wording change: for
external_locationsandvector_search_endpoints, the Detail message now reads... use external_location resources./... use vector_search_endpoint resources.(snake_case, fromSingularName) instead of... use external location resources./... use vector search endpoint resources.(the old struct's hand-written field with spaces). The catalog message is byte-identical. There are no acceptance tests covering the other two messages.Tests
validate_direct_only_resources_test.go(table-driven over the three direct-only types).bundle/validate/catalog_requires_direct_modepasses byte-identically.bundle/destroy/...,bundle/deploy/...(excluding the pre-existingspark-jar-taskJava env failure that also fails onmain),bundle/resources/grants/schemas/...,bundle/config-remote-sync/..., andbundle/user_agent/simpleall pass byte-identically../task fmt,./task checks,./task lintclean.This PR was written by Claude Code.