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

Fix panic when bundle auth resolution fails #1002

Merged
merged 14 commits into from
Nov 30, 2023

Conversation

shreyas-goenka
Copy link
Contributor

@shreyas-goenka shreyas-goenka commented Nov 21, 2023

Changes

CLI would panic if an invalid bundle auth is setup when running CLI commands. This PR removes the panic and shows the error message directly instead.

Tests

The CWD is a bundle with:

workspace:
  profile: DEFAULT

Before:

shreyas.goenka@THW32HFW6T bundle-playground % cli clusters list
panic: resolve: /Users/shreyas.goenka/.databrickscfg has no DEFAULT profile configured. Config: profile=DEFAULT

goroutine 1 [running]:

After:

shreyas.goenka@THW32HFW6T bundle-playground % cli clusters list
Error: cannot resolve bundle auth configuration: resolve: /Users/shreyas.goenka/.databrickscfg has no DEFAULT profile configured. Config: profile=DEFAULT
shreyas.goenka@THW32HFW6T bundle-playground % DATABRICKS_CONFIG_FILE=/dev/null cli bundle deploy
Error:  cannot resolve bundle auth configuration: resolve: /dev/null has no DEFAULT profile configured. Config: profile=DEFAULT, config_file=/dev/null. Env: DATABRICKS_CONFIG_FILE

bundle/bundle.go Outdated Show resolved Hide resolved
bundle/bundle.go Outdated Show resolved Hide resolved
cmd/root/auth.go Outdated Show resolved Hide resolved
@shreyas-goenka shreyas-goenka requested review from pietern and andrewnester and removed request for pietern and andrewnester November 27, 2023 14:23
@shreyas-goenka
Copy link
Contributor Author

Fixing failing tests

@shreyas-goenka shreyas-goenka requested review from pietern and andrewnester and removed request for pietern and andrewnester November 27, 2023 14:35
}
return client, nil
}

func (b *Bundle) WorkspaceClient() *databricks.WorkspaceClient {
b.clientOnce.Do(func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we really need b.clientOnce.Do still? Now b.client initialised either in bundle mutator or in MustWorkspaceClient.
I'd expect we don't use WorkspaceClient() outside of these 2 context, so we can simplify it to

func (b *Bundle) WorkspaceClient() *databricks.WorkspaceClient {
   if b.client == nil {
     panic("Oh no, not expected")
   }
   return b.client
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. There was also the databricks labs command group which uses a client without triggering the initialize mutator. Now also calling the initialize function from the databricks labs command.

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 reverted this. Let's keep clientOnce. Removing it is not trivial since a bunch of tests depend upon it. One option would have been to make the InitialiseWorkspaceClient a DefaultMutator(), but unfortunately target resolution happens after default mutators are applied, and we need to initialize the workspace client after target resolution (because target could contain different auth credentials).

Alternately we could call InitializeWorkspaceClient from all tests that call b.WorkspaceClient() but that's a bigger lift and arguablely not worth it.

WDYT?

@shreyas-goenka shreyas-goenka added this pull request to the merge queue Nov 30, 2023
Merged via the queue into main with commit 677926b Nov 30, 2023
4 checks passed
@shreyas-goenka shreyas-goenka deleted the fix-invalid-bundle-auth branch November 30, 2023 14:33
andrewnester added a commit that referenced this pull request Dec 6, 2023
CLI:
 * Add documentation for positional args in commands generated from the Databricks OpenAPI specification ([#1033](#1033)).
 * Ask for host when .databrickscfg doesn't exist ([#1041](#1041)).
 * Add list of supported values for flags that represent an enum field ([#1036](#1036)).

Bundles:
 * Fix panic when bundle auth resolution fails ([#1002](#1002)).
 * Add versioning for bundle templates ([#972](#972)).
 * Add support for conditional prompting in bundle init ([#971](#971)).
 * Pass parameters to task when run with `--python-params` and `python_wheel_wrapper` is true ([#1037](#1037)).
 * Change default_python template to auto-update version on each wheel build ([#1034](#1034)).

Internal:
 * Rewrite the friendly log handler ([#1038](#1038)).
 * Move bundle schema update to an internal module ([#1012](#1012)).

Dependency updates:
 * Bump github.com/databricks/databricks-sdk-go from 0.26.0 to 0.26.1 ([#1040](#1040)).
@andrewnester andrewnester mentioned this pull request Dec 6, 2023
github-merge-queue bot pushed a commit that referenced this pull request Dec 6, 2023
CLI:
* Add documentation for positional args in commands generated from the
Databricks OpenAPI specification
([#1033](#1033)).
* Ask for host when .databrickscfg doesn't exist
([#1041](#1041)).
* Add list of supported values for flags that represent an enum field
([#1036](#1036)).

Bundles:
* Fix panic when bundle auth resolution fails
([#1002](#1002)).
* Add versioning for bundle templates
([#972](#972)).
* Add support for conditional prompting in bundle init
([#971](#971)).
* Pass parameters to task when run with `--python-params` and
`python_wheel_wrapper` is true
([#1037](#1037)).
* Change default_python template to auto-update version on each wheel
build ([#1034](#1034)).

Internal:
* Rewrite the friendly log handler
([#1038](#1038)).
* Move bundle schema update to an internal module
([#1012](#1012)).


Dependency updates:
* Bump github.com/databricks/databricks-sdk-go from 0.26.0 to 0.26.1
([#1040](#1040)).
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.

None yet

3 participants