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

Support multiple locations for diagnostics #1610

Merged
merged 19 commits into from
Jul 23, 2024
Merged

Conversation

shreyas-goenka
Copy link
Contributor

@shreyas-goenka shreyas-goenka commented Jul 18, 2024

Changes

This PR changes diag.Diagnostics to allow including multiple locations associated with the diagnostic message. The diagnostics that now return multiple locations with this PR are:

  1. Warning for unknown keys in config.
  2. Use of experimental.run_as
  3. Accidental sync.exludes that exclude all files.

Tests

Existing unit tests pass. New unit test case to assert on error message when multiple locations are included.

Example output:

➜  bundle-playground-2 ~/cli2/cli/cli bundle validate              
Warning: You are using the legacy mode of run_as. The support for this mode is experimental and might be removed in a future release of the CLI. In order to run the DLT pipelines in your DAB as the run_as user this mode changes the owners of the pipelines to the run_as identity, which requires the user deploying the bundle to be a workspace admin, and also a Metastore admin if the pipeline target is in UC.
  at experimental.use_legacy_run_as
  in resources.yml:10:22
     databricks.yml:13:22

Name: fix run_if
Target: default
Workspace:
  User: shreyas.goenka@databricks.com
  Path: /Users/shreyas.goenka@databricks.com/.bundle/fix run_if/default

Found 1 warning

@shreyas-goenka
Copy link
Contributor Author

Just FYI, gopatch used to get refactor and get to a starting point for more custom changes. There's something satisfying about writing patch files:

@@
var t expression
@@
diag.Diagnostic{
    ...,
-   Locations: t,
+   Locations: []dyn.Location{t},
    ...,
}


@@
@@
diag.Diagnostics{
    {
        ...,
-       Locations: dyn.Location{...},
+       Locations: []dyn.Location{{...}},
    },
}

@@
@@
package python

-assert.Equal(t, dyn.Location{...}, diags[0].Locations)
+assert.Equal(t, []dyn.Location{{...}}, diags[0].Locations)

@@
@@
-b.Config.GetLocation("experimental.use_legacy_run_as")
+b.Config.GetLocations("experimental.use_legacy_run_as")

@@
var x identifier
@@
package validate

diag.Diagnostic{
    ...,
-   Locations: []dyn.Location{x.Location()},
+   Locations: x.Locations(),
    ...,
}

@@
@@
-[]dyn.Location{dyn.Location{...}}
+[]dyn.Location{{...}}

@shreyas-goenka
Copy link
Contributor Author

shreyas-goenka commented Jul 19, 2024

Integration tests have been triggered, just in case.

Severity: diag.Warning,
Summary: "You are using the legacy mode of run_as. The support for this mode is experimental and might be removed in a future release of the CLI. In order to run the DLT pipelines in your DAB as the run_as user this mode changes the owners of the pipelines to the run_as identity, which requires the user deploying the bundle to be a workspace admin, and also a Metastore admin if the pipeline target is in UC.",
Path: dyn.MustPathFromString("experimental.use_legacy_run_as"),
Locations: b.Config.GetLocations("experimental.use_legacy_run_as"),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that we display multiple locations here since they are all relevant.

Path: loc.Path(),
Severity: diag.Warning,
Summary: fmt.Sprintf("Pattern %s does not match any files", p),
Locations: []dyn.Location{loc.Location()},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does not matter if we use loc.Locations() or just a single location here since array values can only have a single location during merging. Still only using a single location here to keep it easy to reason about.

// Show only the location where the job_cluster_key is defined.
// Other associated locations are not relevant since they are
// overridden during merging.
Locations: []dyn.Location{loc.Location()},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eventually I'll followup with a PR to show warnings when scalar configuration is being overridden.

@shreyas-goenka shreyas-goenka marked this pull request as ready for review July 19, 2024 12:50
bundle/config/mutator/python/python_diagnostics_test.go Outdated Show resolved Hide resolved
bundle/config/root.go Show resolved Hide resolved
bundle/render/render_text_output.go Show resolved Hide resolved
libs/dyn/convert/normalize.go Show resolved Hide resolved
@shreyas-goenka shreyas-goenka requested a review from pietern July 23, 2024 17:06
@shreyas-goenka
Copy link
Contributor Author

Also addressing the applicable comments in #1616

@shreyas-goenka shreyas-goenka added this pull request to the merge queue Jul 23, 2024
Merged via the queue into main with commit 4bf88b4 Jul 23, 2024
5 checks passed
@shreyas-goenka shreyas-goenka deleted the diag-multiple-locations branch July 23, 2024 17:34
andrewnester added a commit that referenced this pull request Jul 25, 2024
Bundles:
 * Add UUID function to bundle template functions  ([#1612](#1612)).
 * Upgrade TF provider to 1.49.0 ([#1617](#1617)).
 * Upgrade TF provider to 1.49.1 ([#1626](#1626)).
 * Support multiple locations for diagnostics ([#1610](#1610)).
 * Split artifact cleanup into prepare step before build ([#1618](#1618)).
 * Move to a single prompt during bundle destroy ([#1583](#1583)).

Internal:
 * Add tests for the Workspace API readahead cache ([#1605](#1605)).
 * Update Python dependencies before install when upgrading a labs project ([#1624](#1624)).
@andrewnester andrewnester mentioned this pull request Jul 25, 2024
github-merge-queue bot pushed a commit that referenced this pull request Jul 26, 2024
Bundles:
* Add UUID function to bundle template functions
([#1612](#1612)).
* Upgrade TF provider to 1.49.0
([#1617](#1617)).
* Upgrade TF provider to 1.49.1
([#1626](#1626)).
* Support multiple locations for diagnostics
([#1610](#1610)).
* Split artifact cleanup into prepare step before build
([#1618](#1618)).
* Move to a single prompt during bundle destroy
([#1583](#1583)).

Internal:
* Add tests for the Workspace API readahead cache
([#1605](#1605)).
* Update Python dependencies before install when upgrading a labs
project ([#1624](#1624)).
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