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

Print diagnostics in 'bundle deploy' #1579

Merged
merged 3 commits into from
Jul 10, 2024

Conversation

kanterov
Copy link
Collaborator

@kanterov kanterov commented Jul 8, 2024

Changes

Print diagnostics in 'bundle deploy' similar to 'bundle validate'. This way if a bundle has any errors or warnings, they are going to be easy to notice.

NB: due to how we render errors, there is one extra trailing new line in output, preserved in examples below

Example: No errors or warnings

% databricks bundle deploy
Building default...
Deploying resources...
Updating deployment state...
Deployment complete!

Example: Error on load

% databricks bundle deploy
Error: Databricks CLI version constraint not satisfied. Required: >= 1337.0.0, current: 0.0.0-dev

Example: Warning on load

% databricks bundle deploy
Building default...
Deploying resources...
Updating deployment state...
Deployment complete!
Warning: unknown field: foo
  in databricks.yml:6:1

Example: Error + warning on load

% databricks bundle deploy
Warning: unknown field: foo
  in databricks.yml:6:1

Error: something went wrong

Example: Warning on load + error in init

% databricks bundle deploy
Warning: unknown field: foo
  in databricks.yml:6:1

Error: Failed to xxx
  in yyy.yml

Detailed explanation
in multiple lines

Tests

Tested manually

@kanterov kanterov requested a review from pietern July 8, 2024 13:24
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.

This is a good step, but only half of a solution.

The validate command doesn't have side effects, so you can naturally decide whether to address the warnings or run with them. This is not the case for deploy, where we'll continue and do the actual deployment regardless of warnings.

Let's pair this with other changes that:

  1. Make it possible to treat warnings as errors
  2. Can stream warnings as they happen as opposed to delaying them until the end of execution

@pietern pietern requested a review from shreyas-goenka July 10, 2024 06:33
@kanterov
Copy link
Collaborator Author

@pietern I agree with your points. Do you think this is something we can do as a follow-up? With this change, we don't change the existing execution flow, and the points you mentioned are already true. However, it addresses the problem of users not being aware of warnings at all and adds pretty-printing of errors/warnings, which is useful when they have source location and additional details.

@pietern
Copy link
Contributor

pietern commented Jul 10, 2024

I triggered nightlies on this change to confirm they fail for the lack of exit status.

If they don't we'll need to address that as well.

@kanterov
Copy link
Collaborator Author

@pietern updated PR description with more examples, there is 1 extra trailing new line, which would be hard to get rid of due to how we use go template, unless we do extra buffering

@kanterov kanterov requested a review from pietern July 10, 2024 10:35
@pietern
Copy link
Contributor

pietern commented Jul 10, 2024

We can revisit the exact output and make those newlines disappear later.

@pietern pietern added this pull request to the merge queue Jul 10, 2024
Merged via the queue into databricks:main with commit af975ca Jul 10, 2024
5 checks passed
andrewnester added a commit that referenced this pull request Jul 10, 2024
Bundles:
 * Override complex variables with target overrides instead of merging ([#1567](#1567)).
 * Rewrite local path for libraries in foreach tasks ([#1569](#1569)).
 * Change SetVariables mutator to mutate dynamic configuration instead ([#1573](#1573)).
 * Return early in bundle destroy if no deployment exists ([#1581](#1581)).
 * Let notebook detection code use underlying metadata if available ([#1574](#1574)).
 * Remove schema override for variable default value ([#1536](#1536)).
 * Print diagnostics in 'bundle deploy' ([#1579](#1579)).

Internal:
 * Update actions/upload-artifact to v4 ([#1559](#1559)).
 * Use Go 1.22 to build and test ([#1562](#1562)).
 * Move bespoke status call to main workspace files filer ([#1570](#1570)).
 * Add new template ([#1578](#1578)).
 * Add regression tests for CLI error output ([#1566](#1566)).

Dependency updates:
 * Bump golang.org/x/mod from 0.18.0 to 0.19.0 ([#1576](#1576)).
 * Bump golang.org/x/term from 0.21.0 to 0.22.0 ([#1577](#1577)).
@andrewnester andrewnester mentioned this pull request Jul 10, 2024
github-merge-queue bot pushed a commit that referenced this pull request Jul 10, 2024
Bundles:
* Override complex variables with target overrides instead of merging
([#1567](#1567)).
* Rewrite local path for libraries in foreach tasks
([#1569](#1569)).
* Change SetVariables mutator to mutate dynamic configuration instead
([#1573](#1573)).
* Return early in bundle destroy if no deployment exists
([#1581](#1581)).
* Let notebook detection code use underlying metadata if available
([#1574](#1574)).
* Remove schema override for variable default value
([#1536](#1536)).
* Print diagnostics in 'bundle deploy'
([#1579](#1579)).

Internal:
* Update actions/upload-artifact to v4
([#1559](#1559)).
* Use Go 1.22 to build and test
([#1562](#1562)).
* Move bespoke status call to main workspace files filer
([#1570](#1570)).
* Add new template
([#1578](#1578)).
* Add regression tests for CLI error output
([#1566](#1566)).

Dependency updates:
* Bump golang.org/x/mod from 0.18.0 to 0.19.0
([#1576](#1576)).
* Bump golang.org/x/term from 0.21.0 to 0.22.0
([#1577](#1577)).
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.

4 participants