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.

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

Conversation

andrewnester
Copy link
Contributor

Changes

There's no value in building artifacts on destroy because they are just removed from workspace as part of destroy.

@pietern
Copy link
Contributor

pietern commented Nov 5, 2024

Thanks!

Can you double check there is no impact on this:

terraform.StatePull(),
terraform.Interpolate(),
terraform.Write(),
terraform.Plan(terraform.PlanGoal("destroy")),

Since it writes out a TF config (necessary or not?), it is possible that artifact variable references are not resolved.

@andrewnester andrewnester disabled auto-merge November 5, 2024 12:03
@andrewnester
Copy link
Contributor Author

@pietern I brought back resolving artifacts variables just in case for backward comatibility

phases.Build(),
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

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.

Copy link

github-actions bot commented Nov 5, 2024

If integration tests don't run automatically, an authorized user can run them manually by following the instructions below:

Trigger:
go/deco-tests-run/cli

Inputs:

  • PR number: 1882
  • Commit SHA: 99874a2b8ff5541d2813116c84dc58eadb39e8c0

Checks will be approved automatically on success.

@eng-dev-ecosystem-bot
Copy link
Collaborator

Test Details: go/deco-tests/11691811235

Copy link
Contributor

@pietern pietern left a comment

Choose a reason for hiding this comment

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

Note the comment inline. The interpolation can be removed if other changes are made to the definition of destroy and how we write the TF configuration.

Can be done in a later PR.

phases.Build(),
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.

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.

@andrewnester andrewnester added this pull request to the merge queue Nov 7, 2024
Merged via the queue into main with commit 162aa21 Nov 7, 2024
10 checks passed
@andrewnester andrewnester deleted the fix/no-build-on-destroy branch November 7, 2024 09:36
andrewnester added a commit that referenced this pull request Nov 14, 2024
Bundles:
 * Do not execute build on bundle destroy ([#1882](#1882)).
 * Add support for non-Python ipynb notebooks to DABs ([#1827](#1827)).

API Changes:
 * Added `databricks credentials` command group.
 * Changed `databricks genie execute-message-query` command to type `databricks genie execute-message-query` command.
 * Changed `databricks lakeview create` command with new required argument order.
 * Added `databricks aibi-dashboard-embedding-access-policy` command group.
 * Added `databricks aibi-dashboard-embedding-approved-domains` command group.
 * Removed `databricks clean-rooms` command group.

OpenAPI commit d25296d2f4aa7bd6195c816fdf82e0f960f775da (2024-11-07)
Dependency updates:
 * Upgrade TF provider to 1.58.0 ([#1900](#1900)).
 * Bump golang.org/x/sync from 0.8.0 to 0.9.0 ([#1892](#1892)).
 * Bump golang.org/x/text from 0.19.0 to 0.20.0 ([#1893](#1893)).
 * Bump golang.org/x/mod from 0.21.0 to 0.22.0 ([#1895](#1895)).
 * Bump golang.org/x/oauth2 from 0.23.0 to 0.24.0 ([#1894](#1894)).
 * Bump github.com/databricks/databricks-sdk-go from 0.49.0 to 0.51.0 ([#1878](#1878)).
github-merge-queue bot pushed a commit that referenced this pull request Nov 14, 2024
Bundles:
* Do not execute build on bundle destroy
([#1882](#1882)).
* Add support for non-Python ipynb notebooks to DABs
([#1827](#1827)).

API Changes:
 * Added `databricks credentials` command group.
* Changed `databricks lakeview create` command with new required
argument order.

OpenAPI commit d25296d2f4aa7bd6195c816fdf82e0f960f775da (2024-11-07)
Dependency updates:
* Upgrade TF provider to 1.58.0
([#1900](#1900)).
* Bump golang.org/x/sync from 0.8.0 to 0.9.0
([#1892](#1892)).
* Bump golang.org/x/text from 0.19.0 to 0.20.0
([#1893](#1893)).
* Bump golang.org/x/mod from 0.21.0 to 0.22.0
([#1895](#1895)).
* Bump golang.org/x/oauth2 from 0.23.0 to 0.24.0
([#1894](#1894)).
* Bump github.com/databricks/databricks-sdk-go from 0.49.0 to 0.51.0
([#1878](#1878)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants