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

Add validation mutator for volume artifact_path #2050

Merged
merged 19 commits into from
Jan 2, 2025

Conversation

shreyas-goenka
Copy link
Contributor

@shreyas-goenka shreyas-goenka commented Dec 30, 2024

Changes

This PR:

  1. Incrementally improves the error messages shown to the user when the volume they are referring to in workspace.artifact_path does not exist.
  2. Performs this validation in both bundle validate and bundle deploy compared to before on just deployments.
  3. It runs "fast" validations on bundle deploy, which earlier were only run on bundle validate.

Tests

Unit tests and manually. Also, existing integration tests provide coverage (TestUploadArtifactToVolumeNotYetDeployed, TestUploadArtifactFileToVolumeThatDoesNotExist)

Examples:

.venv➜  bundle-playground git:(master) ✗ cli bundle validate
Error: cannot access volume capital.whatever.my_volume: User does not have READ VOLUME on Volume 'capital.whatever.my_volume'.
  at workspace.artifact_path
  in databricks.yml:7:18

and

.venv➜  bundle-playground git:(master) ✗ cli bundle validate
Error: volume capital.whatever.foobar does not exist
  at workspace.artifact_path
     resources.volumes.foo
  in databricks.yml:7:18
     databricks.yml:12:7

You are using a volume in your artifact_path that is managed by
this bundle but which has not been deployed yet. Please first deploy
the volume using 'bundle deploy' and then switch over to using it in
the artifact_path.

@denik denik changed the title Add validation mutator for volume arifact_path Add validation mutator for volume artifact_path Dec 30, 2024
CreateVolumeRequestContent: &catalog.CreateVolumeRequestContent{
CatalogName: "catalogN",
Name: "volumeN",
VolumeType: "MANAGED",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the VolumeType necessary for this test?

// Fast mutators with only in-memory checks
JobClusterKeyDefined(),
JobTaskClusterSpec(),
SingleNodeCluster(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Do all of these return errors?

Warnings currently only show up at the end of deploy, so they're not actionable (yet).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, SingleNodeCluster() returns a warning that is useful even with a deployment. The others, though indeed, are no longer useful.

I can follow up with a PR that serializes the warnings (and below like INFO) on a rolling basis in the bundle.Seq mutator. That should make the warnings useful before deployment.

Copy link
Contributor

Choose a reason for hiding this comment

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

The others, though indeed, are no longer useful.

How so?

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to revisit how we output these diags anyway, so no need to address here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How so?

You'd end up with runtime errors on JobClusterKeyDefined. The validation warning has a runtime error equivalent:

.venv➜  bundle-playground git:(master) ✗ databricks bundle validate
Warning: job_cluster_key c1 is not defined
  at resources.jobs.foo.tasks[0].job_cluster_key
  in databricks.yml:17:28
Error: terraform apply: exit status 1

Error: cannot create job: Job cluster 'c1' is not defined in field 'job_clusters'.

  with databricks_job.foo,
  on bundle.tf.json line 35, in resource.databricks_job.foo:
  35:       }

JobTaskClusterSpec returns errors though so it's useful for returning an error early.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should probably promote this warning to an error though.

Copy link

github-actions bot commented Jan 2, 2025

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: 2050
  • Commit SHA: 8feb04a05ea92510250511d42e89ddc86220ec43

Checks will be approved automatically on success.

@shreyas-goenka shreyas-goenka merged commit 7beb0fb into main Jan 2, 2025
8 of 9 checks passed
@shreyas-goenka shreyas-goenka deleted the validate-artifact-path branch January 2, 2025 11:53
pietern added a commit that referenced this pull request Jan 8, 2025
Bundles:
 * Fix finding Python within virtualenv on Windows ([#2034](#2034)).
 * Include missing field descriptions in JSON schema ([#2045](#2045)).
 * Add validation for volume referenced from `artifact_path` ([#2050](#2050)).
 * Handle `${workspace.file_path}` references in source-linked deployments ([#2046](#2046)).
 * Set the write bit for files written during template initialization ([#2068](#2068)).
github-merge-queue bot pushed a commit that referenced this pull request Jan 8, 2025
Bundles:
* Fix finding Python within virtualenv on Windows
([#2034](#2034)).
* Include missing field descriptions in JSON schema
([#2045](#2045)).
* Add validation for volume referenced from `artifact_path`
([#2050](#2050)).
* Handle `${workspace.file_path}` references in source-linked
deployments ([#2046](#2046)).
* Set the write bit for files written during template initialization
([#2068](#2068)).
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.

2 participants