Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not execute build on bundle destroy #1882

Merged
merged 3 commits into from
Nov 7, 2024
Merged

Do not execute build on bundle destroy #1882

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ac40e02
Do not execute build on bundle destroy
andrewnester Nov 5, 2024
f2ff4db
resolve variables for backward compatibility
andrewnester Nov 5, 2024
99874a2
add a comment
andrewnester Nov 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion cmd/bundle/destroy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"os"

"github.com/databricks/cli/bundle"
"github.com/databricks/cli/bundle/config/mutator"
"github.com/databricks/cli/bundle/phases"
"github.com/databricks/cli/cmd/bundle/utils"
"github.com/databricks/cli/cmd/root"
Expand Down Expand Up @@ -62,7 +63,12 @@ func newDestroyCommand() *cobra.Command {

diags = bundle.Apply(ctx, b, bundle.Seq(
phases.Initialize(),
phases.Build(),
// We need to resolve artifact variable (how we do it in build phase)
// because some of the to-be-destroyed resource might use this variable.
// Not resolving might lead to terraform "Reference to undeclared resource" error
mutator.ResolveVariableReferences(
"artifacts",
),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if this is necessary. I was asking because there may be a subtle dependency we're missing, and I was wondering if you did the analysis on this. If it is necessary, then please include a comment to capture the nuance, otherwise we can skip this altogether.

Copy link
Contributor

@shreyas-goenka shreyas-goenka Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to skip this since terraform destroy depends entirely on the id stored in the state (IIRC). The configuration we write to bundle.tf.json should be irrelevant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If artifact variable is used in some resource, let's say in job name, the destroy will fail with the following error if variables not resolved

    my_job:
      name: My Job at ${artifacts.test.path}

ndrew.nester@HFW9Y94129 wheel % databricks bundle destroy -p u2m
Error: exit status 1

Error: Reference to undeclared resource

  on bundle.tf.json line 45, in resource.databricks_job.my_job:
  45:         "name": "My Job ${artifacts.test.path}",

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add the comment

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true only if we write bundle.tf.json with all resources prior to destroying.

I suspect we can write an empty bundle.tf.json (with only the provider definition) and sidestep the issue.

phases.Destroy(),
))
if err := diags.Error(); err != nil {
Expand Down
Loading