diff --git a/.nextchanges/bundles/select-delete-dependency.md b/.nextchanges/bundles/select-delete-dependency.md new file mode 100644 index 00000000000..11152b3e7f0 --- /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 an error, as it does when 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..696906db46c 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,22 @@ 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. - var errs []string + // nodes pull in their dependencies. Collect conflicts for live dependents of every + // delete visited before returning, so the user sees all conflicts at once. + 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:] 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 @@ -396,18 +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.") - 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 8c6dd30240e..80f57c3e1f5 100644 --- a/bundle/deployplan/plan_filter_test.go +++ b/bundle/deployplan/plan_filter_test.go @@ -269,6 +269,41 @@ 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") +} + +// 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) {