Resolve configuration before performing verification #890
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
If a bundle configuration specifies a workspace host, and the user specifies a profile to use, we perform a check to confirm that the workspace host in the bundle configuration and the workspace host from the profile are identical. If they are not, we return an error. The check was introduced in #571. Previously, the code included an assumption that the client configuration was already loaded from the environment prior to performing the check. This was not the case, and as such if the user intended to use a non-default path to `.databrickscfg`, this path was not used when performing the check. The fix does the following: * Resolve the configuration prior to performing the check. * Don't treat the configuration file not existing as an error. * Add unit tests. Fixes #884.
pietern
commented
Oct 19, 2023
| assert.NoError(t, err) | ||
| assert.Equal(t, "default", cfg.Token) | ||
| } | ||
|
|
There was a problem hiding this comment.
This test is superfluous.
The same is covered by TestLoaderMatchingHost and TestLoaderMatchingHostWithQuery below.
libs/databrickscfg/ops.go
Outdated
| configFile, err := config.LoadFile(cfg.ConfigFile) | ||
| if err != nil { | ||
| if os.IsNotExist(err) { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
This fixes the error if the configuration file doesn't exist.
| @@ -59,7 +59,7 @@ func TestLoaderErrorsOnInvalidFile(t *testing.T) { | |||
| assert.ErrorContains(t, err, "unclosed section: ") | |||
| } | |||
|
|
|||
| func TestLoaderSkipssNoMatchingHost(t *testing.T) { | |||
| func TestLoaderSkipsNoMatchingHost(t *testing.T) { | |||
| @@ -103,6 +103,7 @@ func (l profileFromHostLoader) Configure(cfg *config.Config) error { | |||
| return fmt.Errorf("%s %s profile: %w", configFile.Path(), match.Name(), err) | |||
| } | |||
|
|
|||
| cfg.Profile = match.Name() | |||
There was a problem hiding this comment.
Write back the profile after selecting one so we can assert on it in tests.
| err := cfg.EnsureResolved() | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
After this, all environment variables, and optionally a profile, have been loaded.
libs/databrickscfg/ops.go
Outdated
| configFile, err := config.LoadFile(cfg.ConfigFile) | ||
| if err != nil { | ||
| if os.IsNotExist(err) { | ||
| return nil |
There was a problem hiding this comment.
Not entirely clear what happens if configuration file is not found, are we just skipping this validate and fail at a later point?
There was a problem hiding this comment.
Good point -- it did indeed skip validation, similar to how the SDK allows a profile to be set but doesn't error:
However, the intent here is always to use a profile if it is specified, so it should return an error if we cannot resolve the profile. Addressed this in the next commit.
andrewnester
approved these changes
Oct 20, 2023
pietern
added a commit
that referenced
this pull request
Oct 23, 2023
CLI: * Never load authentication configuration from bundle for sync command ([#889](#889)). * Fixed requiring positional arguments for API URL parameters ([#878](#878)). Bundles: * Add support for validating CLI version when loading a jsonschema object ([#883](#883)). * Do not emit wheel wrapper error when python_wheel_wrapper setting is true ([#894](#894)). * Resolve configuration before performing verification ([#890](#890)). * Fix wheel task not working with with 13.x clusters ([#898](#898)). Internal: * Skip prompt on completion hook ([#888](#888)). * New YAML loader to support configuration location ([#828](#828)). Dependency updates: * Bump github.com/mattn/go-isatty from 0.0.19 to 0.0.20 ([#896](#896)).
Merged
github-merge-queue bot
pushed a commit
that referenced
this pull request
Oct 23, 2023
CLI: * Never load authentication configuration from bundle for sync command ([#889](#889)). * Fixed requiring positional arguments for API URL parameters ([#878](#878)). Bundles: * Add support for validating CLI version when loading a jsonschema object ([#883](#883)). * Do not emit wheel wrapper error when python_wheel_wrapper setting is true ([#894](#894)). * Resolve configuration before performing verification ([#890](#890)). * Fix wheel task not working with with 13.x clusters ([#898](#898)). Internal: * Skip prompt on completion hook ([#888](#888)). * New YAML loader to support configuration location ([#828](#828)). Dependency updates: * Bump github.com/mattn/go-isatty from 0.0.19 to 0.0.20 ([#896](#896)).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
If a bundle configuration specifies a workspace host, and the user specifies a profile to use, we perform a check to confirm that the workspace host in the bundle configuration and the workspace host from the profile are identical. If they are not, we return an error. The check was introduced in #571.
Previously, the code included an assumption that the client configuration was already loaded from the environment prior to performing the check. This was not the case, and as such if the user intended to use a non-default path to
.databrickscfg, this path was not used when performing the check.The fix does the following:
Fixes #884.
Tests
Unit tests and manual confirmation.