From 290054b87a3d22a05c1d965de3e31d6445e616af Mon Sep 17 00:00:00 2001 From: "pieter.noordhuis" Date: Wed, 4 Nov 2026 08:37:13 +0000 Subject: [PATCH 1/2] Check live dependents of deletes reached as dependencies in --select FilterToSelected only built reverse edges when a selected resource was a delete. A delete pulled in through a forward DependsOn edge of a selected resource (e.g. `--select jobs.consumer` where consumer still references a removed jobs.producer) was therefore never checked for live dependents and was deleted, while `--select jobs.consumer,jobs.producer` correctly errored. Build the reverse edges lazily on the first delete visited by the BFS so every delete gets the same live-dependent check. When the offending delete was not itself selected, say so in the error, since the user never named it. Co-authored-by: Isaac --- .../bundles/select-delete-dependency.md | 1 + .../select/deletion_dependents/output.txt | 10 +++++++ .../bundle/select/deletion_dependents/script | 7 ++++- bundle/deployplan/plan.go | 26 ++++++++++--------- bundle/deployplan/plan_filter_test.go | 16 ++++++++++++ 5 files changed, 47 insertions(+), 13 deletions(-) create mode 100644 .nextchanges/bundles/select-delete-dependency.md diff --git a/.nextchanges/bundles/select-delete-dependency.md b/.nextchanges/bundles/select-delete-dependency.md new file mode 100644 index 00000000000..bb16f269898 --- /dev/null +++ b/.nextchanges/bundles/select-delete-dependency.md @@ -0,0 +1 @@ +* Fix `bundle deploy --select` deleting a resource that the selected resource still references when that resource was removed from the config; it now reports the same error as selecting the deleted resource directly. ([#7334](https://github.com/databricks/cli/pull/7334)) diff --git a/acceptance/bundle/select/deletion_dependents/output.txt b/acceptance/bundle/select/deletion_dependents/output.txt index c58c0895137..0df55ddb3b4 100644 --- a/acceptance/bundle/select/deletion_dependents/output.txt +++ b/acceptance/bundle/select/deletion_dependents/output.txt @@ -19,6 +19,16 @@ Error: cannot delete jobs.producer: jobs.consumer still references it; remove th Files: 1 uploaded, 0 deleted +>>> [CLI] bundle plan --select jobs.consumer +Error: cannot delete jobs.producer (not selected, but required by the selection): jobs.consumer still references it; remove the reference or also remove jobs.consumer from the config + + +>>> [CLI] bundle deploy --select jobs.consumer +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/select-deldep-[UNIQUE_NAME]/default/files... +Error: cannot delete jobs.producer (not selected, but required by the selection): jobs.consumer still references it; remove the reference or also remove jobs.consumer from the config + +Files: 0 uploaded, 0 deleted + >>> print_requests.py --sort //jobs/delete --keep >>> [CLI] bundle plan diff --git a/acceptance/bundle/select/deletion_dependents/script b/acceptance/bundle/select/deletion_dependents/script index d2c1dab89f5..cf2c5750467 100644 --- a/acceptance/bundle/select/deletion_dependents/script +++ b/acceptance/bundle/select/deletion_dependents/script @@ -23,7 +23,12 @@ musterr trace $CLI bundle plan --select jobs.producer # bundle deploy --select also errors before sending any delete request. musterr trace $CLI bundle deploy --select jobs.producer -# No delete request was sent by either command. +# Selecting only consumer pulls in producer as a dependency. That delete would +# leave consumer broken just the same, so it must error too. +musterr trace $CLI bundle plan --select jobs.consumer +musterr trace $CLI bundle deploy --select jobs.consumer + +# No delete request was sent by any of these commands. trace print_requests.py --sort //jobs/delete --keep # A full plan still works and shows producer as a pending delete. diff --git a/bundle/deployplan/plan.go b/bundle/deployplan/plan.go index 8446b200b12..a8233c4e669 100644 --- a/bundle/deployplan/plan.go +++ b/bundle/deployplan/plan.go @@ -340,8 +340,9 @@ func ResolveSelectors(resourceKeys iter.Seq[string], selectors []string) ([]stri // ResolveSelectors) plus the others that must change with them. The closure follows // edges by action, like the apply graph (makeGraph): a deployed resource pulls in its // dependencies (forward), a deleted one its dependents (reverse) so they are removed -// together. A delete selection errors if a live resource (not itself being deleted) -// still references the resource to be deleted; removing it would break the live resource. +// together. It errors if any delete in the closure, whether selected or pulled in by +// either edge direction, is still referenced by a live resource (not itself being +// deleted); removing it would break the live resource. func (p *Plan) FilterToSelected(_ context.Context, selected []string) error { before := len(p.Plan) @@ -350,15 +351,9 @@ func (p *Plan) FilterToSelected(_ context.Context, selected []string) error { return err } - // Only build reverse edges when at least one selected resource is being deleted, - // since non-delete selections only follow forward DependsOn edges. + // Reverse edges are built lazily on the first delete visited by the BFS. That + // delete may be selected or reached as a dependency of a selected resource. var dependents map[string][]string - for _, key := range resolved { - if e, ok := p.Plan[key]; ok && e.Action == Delete { - dependents = p.reverseEdges() - break - } - } queue := make([]string, 0, len(selected)) reachable := make(map[string]struct{}, len(selected)) @@ -374,13 +369,16 @@ func (p *Plan) FilterToSelected(_ context.Context, selected []string) error { } // BFS: deleted nodes pull in their dependents (also being deleted); deployed - // nodes pull in their dependencies. Collect errors for live dependents of a - // delete selection before returning, so the user sees all conflicts at once. + // nodes pull in their dependencies. Collect errors for live dependents of every + // delete visited before returning, so the user sees all conflicts at once. var errs []string for len(queue) > 0 { key := queue[0] queue = queue[1:] if p.Plan[key].Action == Delete { + if dependents == nil { + dependents = p.reverseEdges() + } for _, dependent := range dependents[key] { // Sub-nodes (grants/permissions, 4-part key) are not user-facing dependents // and never trigger the live-dependent error. If they are themselves being @@ -399,6 +397,10 @@ func (p *Plan) FilterToSelected(_ context.Context, selected []string) error { // it broken. Report a user-facing error with both names. deleted := strings.TrimPrefix(key, "resources.") live := strings.TrimPrefix(dependent, "resources.") + // Without this, an error about a resource the user never named is confusing. + if !slices.Contains(resolved, key) { + deleted += " (not selected, but required by the selection)" + } errs = append(errs, fmt.Sprintf("cannot delete %s: %s still references it; remove the reference or also remove %s from the config", deleted, live, live)) } } diff --git a/bundle/deployplan/plan_filter_test.go b/bundle/deployplan/plan_filter_test.go index 8c6dd30240e..df419f202dd 100644 --- a/bundle/deployplan/plan_filter_test.go +++ b/bundle/deployplan/plan_filter_test.go @@ -269,6 +269,22 @@ func TestFilterToSelected_DeletionErrorsOnLiveDependent(t *testing.T) { assert.ErrorContains(t, err, "jobs.live still references it") } +// A delete reached through a forward dependency edge is subject to the same +// live-dependent check as a selected delete. +func TestFilterToSelected_DependencyDeletionErrorsOnLiveDependent(t *testing.T) { + p := deployplan.NewPlanDirect() + p.Plan["resources.jobs.gone"] = &deployplan.PlanEntry{Action: deployplan.Delete} + p.Plan["resources.jobs.live"] = &deployplan.PlanEntry{ + Action: deployplan.Update, + DependsOn: []deployplan.DependsOnEntry{{Node: "resources.jobs.gone"}}, + } + + err := p.FilterToSelected(t.Context(), []string{"jobs.live"}) + require.Error(t, err) + assert.ErrorContains(t, err, "cannot delete jobs.gone (not selected, but required by the selection)") + assert.ErrorContains(t, err, "jobs.live still references it") +} + // Selecting a resource for deletion leaves its own dependencies alone: they are not // pulled into the plan, so they stay deployed untouched. func TestFilterToSelected_DeletionLeavesDependencies(t *testing.T) { From 04bce2306d9109ee1e2c085fc55569c1de6c98fa Mon Sep 17 00:00:00 2001 From: "pieter.noordhuis" Date: Wed, 4 Nov 2026 16:03:43 +0000 Subject: [PATCH 2/2] Mark only forward-edge deletes as required by the selection The "(not selected, but required by the selection)" suffix was also added to deletes pulled in through the cascade (reverse) edge of a selected delete. Those keep the plain error message again. The suffix is now decided after the BFS, since a delete can be reached through both edge directions and the forward route may be found after it was visited. Also reword the changelog entry, which no longer reports the exact same error as selecting the deleted resource directly. Co-authored-by: Isaac --- .../bundles/select-delete-dependency.md | 2 +- bundle/deployplan/plan.go | 35 +++++++++++++------ bundle/deployplan/plan_filter_test.go | 19 ++++++++++ 3 files changed, 45 insertions(+), 11 deletions(-) diff --git a/.nextchanges/bundles/select-delete-dependency.md b/.nextchanges/bundles/select-delete-dependency.md index bb16f269898..11152b3e7f0 100644 --- a/.nextchanges/bundles/select-delete-dependency.md +++ b/.nextchanges/bundles/select-delete-dependency.md @@ -1 +1 @@ -* Fix `bundle deploy --select` deleting a resource that the selected resource still references when that resource was removed from the config; it now reports the same error as selecting the deleted resource directly. ([#7334](https://github.com/databricks/cli/pull/7334)) +* Fix `bundle deploy --select` deleting a resource that the selected resource still references when that resource was removed from the config; it now reports an error, as it does when selecting the deleted resource directly. ([#7334](https://github.com/databricks/cli/pull/7334)) diff --git a/bundle/deployplan/plan.go b/bundle/deployplan/plan.go index a8233c4e669..696906db46c 100644 --- a/bundle/deployplan/plan.go +++ b/bundle/deployplan/plan.go @@ -369,9 +369,15 @@ func (p *Plan) FilterToSelected(_ context.Context, selected []string) error { } // BFS: deleted nodes pull in their dependents (also being deleted); deployed - // nodes pull in their dependencies. Collect errors for live dependents of every + // nodes pull in their dependencies. Collect conflicts for live dependents of every // delete visited before returning, so the user sees all conflicts at once. - var errs []string + type conflict struct{ deleted, live string } + var conflicts []conflict + // Unselected deletes reached through a deployed node's forward edge. Errors for + // them are marked as such, since the user never named them. Deletes reached + // through the reverse (cascade) edge keep the plain message: there the error is + // about the cascade, not about the selection requiring the delete. + requiredBySelection := map[string]bool{} for len(queue) > 0 { key := queue[0] queue = queue[1:] @@ -394,22 +400,31 @@ func (p *Plan) FilterToSelected(_ context.Context, selected []string) error { p.enqueueReachable(reachable, &queue, dependent) } else { // Dependent is live (still in config): deleting key would leave - // it broken. Report a user-facing error with both names. - deleted := strings.TrimPrefix(key, "resources.") - live := strings.TrimPrefix(dependent, "resources.") - // Without this, an error about a resource the user never named is confusing. - if !slices.Contains(resolved, key) { - deleted += " (not selected, but required by the selection)" - } - errs = append(errs, fmt.Sprintf("cannot delete %s: %s still references it; remove the reference or also remove %s from the config", deleted, live, live)) + // it broken. + conflicts = append(conflicts, conflict{deleted: key, live: dependent}) } } } else { for _, dep := range p.Plan[key].DependsOn { + if entry, ok := p.Plan[dep.Node]; ok && entry.Action == Delete && !slices.Contains(resolved, dep.Node) { + requiredBySelection[dep.Node] = true + } p.enqueueReachable(reachable, &queue, dep.Node) } } } + + // Format after the BFS: a delete can be reached through both edge directions, + // and the forward route may be found only after the delete was visited. + errs := make([]string, 0, len(conflicts)) + for _, c := range conflicts { + deleted := strings.TrimPrefix(c.deleted, "resources.") + live := strings.TrimPrefix(c.live, "resources.") + if requiredBySelection[c.deleted] { + deleted += " (not selected, but required by the selection)" + } + errs = append(errs, fmt.Sprintf("cannot delete %s: %s still references it; remove the reference or also remove %s from the config", deleted, live, live)) + } if len(errs) > 0 { slices.Sort(errs) return errors.New(strings.Join(errs, "\n")) diff --git a/bundle/deployplan/plan_filter_test.go b/bundle/deployplan/plan_filter_test.go index df419f202dd..80f57c3e1f5 100644 --- a/bundle/deployplan/plan_filter_test.go +++ b/bundle/deployplan/plan_filter_test.go @@ -285,6 +285,25 @@ func TestFilterToSelected_DependencyDeletionErrorsOnLiveDependent(t *testing.T) assert.ErrorContains(t, err, "jobs.live still references it") } +// A delete reached through the cascade (reverse) edge of a selected delete keeps the +// plain error: it is not required by the selection, it is removed along with it. +func TestFilterToSelected_CascadeDeletionErrorsOnLiveDependent(t *testing.T) { + p := deployplan.NewPlanDirect() + p.Plan["resources.jobs.a"] = &deployplan.PlanEntry{Action: deployplan.Delete} + p.Plan["resources.jobs.b"] = &deployplan.PlanEntry{ + Action: deployplan.Delete, + DependsOn: []deployplan.DependsOnEntry{{Node: "resources.jobs.a"}}, + } + p.Plan["resources.jobs.c"] = &deployplan.PlanEntry{ + Action: deployplan.Update, + DependsOn: []deployplan.DependsOnEntry{{Node: "resources.jobs.b"}}, + } + + err := p.FilterToSelected(t.Context(), []string{"jobs.a"}) + require.Error(t, err) + assert.EqualError(t, err, "cannot delete jobs.b: jobs.c still references it; remove the reference or also remove jobs.c from the config") +} + // Selecting a resource for deletion leaves its own dependencies alone: they are not // pulled into the plan, so they stay deployed untouched. func TestFilterToSelected_DeletionLeavesDependencies(t *testing.T) {