-
Notifications
You must be signed in to change notification settings - Fork 71
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
Conversation
…ction and modify all diagnostics paths to be relative to the bundle root path
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:
|
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"), |
There was a problem hiding this comment.
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()}, |
There was a problem hiding this comment.
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()}, |
There was a problem hiding this comment.
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.
Also addressing the applicable comments in #1616 |
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)).
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)).
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:Tests
Existing unit tests pass. New unit test case to assert on error message when multiple locations are included.
Example output: