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.

Sign up for GitHub

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

Make databricks configure save only explicit fields #973

Merged
merged 4 commits into from
Nov 10, 2023
+78 −3
Merged

Make databricks configure save only explicit fields #973

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
656d2f5
Save only explicit config fields to the profile file
ilia-db Nov 9, 2023
fe3f415
Fix formatting
ilia-db Nov 9, 2023
dfab68a
Rename a spec back to its original name
ilia-db Nov 10, 2023
80058a3
Test that `configure` can read host and token from the env vars
ilia-db Nov 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions cmd/auth/login.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,8 +111,13 @@ func newLoginCommand(persistentAuth *auth.PersistentAuth) *cobra.Command {
}

if profileName != "" {
cfg.Profile = profileName
err = databrickscfg.SaveToProfile(ctx, &cfg)
err = databrickscfg.SaveToProfile(ctx, &config.Config{
Profile: profileName,
Host: cfg.Host,
AuthType: cfg.AuthType,
AccountID: cfg.AccountID,
ClusterID: cfg.ClusterID,
})
if err != nil {
return err
}
Expand Down
6 changes: 5 additions & 1 deletion cmd/configure/configure.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,11 @@ func newConfigureCommand() *cobra.Command {
cfg.DatabricksCliPath = ""

// Save profile to config file.
return databrickscfg.SaveToProfile(ctx, &cfg)
return databrickscfg.SaveToProfile(ctx, &config.Config{
Profile: cfg.Profile,
Host: cfg.Host,
Token: cfg.Token,
})
}

return cmd
Expand Down
41 changes: 40 additions & 1 deletion cmd/configure/configure_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,46 @@ func TestConfigFileFromEnvNoInteractive(t *testing.T) {
assertKeyValueInSection(t, defaultSection, "token", "token")
}

func TestCustomProfileConfigureNoInteractive(t *testing.T) {
func TestEnvVarsConfigureNoInteractive(t *testing.T) {
ctx := context.Background()
tempHomeDir := setup(t)
cfgPath := filepath.Join(tempHomeDir, ".databrickscfg")
inp := getTempFileWithContent(t, tempHomeDir, "token\n")
defer inp.Close()
oldStdin := os.Stdin
t.Cleanup(func() { os.Stdin = oldStdin })
os.Stdin = inp

t.Setenv("DATABRICKS_HOST", "https://host")
t.Setenv("DATABRICKS_AUTH_TYPE", "metadata-service")
t.Setenv("DATABRICKS_METADATA_SERVICE_URL", "https://metadata")
Copy link
Contributor

Choose a reason for hiding this comment

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

Please also set the DATABRICKS_TOKEN env var to some value to make sure it doesn't have precedence over the value that the user provides through stdin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the current logic it will pick up the token from the env var and won't even try to ask for a new one from a user (or read from sdtin). From git blame I figured it is a feature, as it was added explicitly - the CLI is able to bootstrap the config based on the environment variables, and if there's enough of them, it will pretty much create a config automatically without any additional prompts.

To me it feels a bit counter intuitive, I would expect databricks configure to always ask all of its inputs. With the current logic it should be named databrikcs write-to-config instead, not configure (since in some cases you don't even configure anything).

Would it make sense to remove reading from the environment completely?

cli/cmd/configure/configure.go

Line 140 in 3284a8c

err := config.ConfigAttributes.Configure(&cfg)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And a side note: databricks vscode extension doesn't set the token env variable, only host with metadata

Copy link
Contributor

Choose a reason for hiding this comment

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

There will be users who set these env vars and expect it to work (predominantly for automation), so let's keep it in tact. I have seen cases where the token is passed through stdin, the rest being set through env vars. Looking at the test context I figured the other env vars would also be masked out, but since it makes sense, disregard my comment.

Do we already have a test confirming that this behavior works? I.e. pass the host and token as env vars and the command silently succeeds and writes out a profile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a test to confirm that configure works with both token and host taken from the env vars


cmd := cmd.New(ctx)
cmd.SetArgs([]string{"configure", "--token"})

err := cmd.ExecuteContext(ctx)
assert.NoError(t, err)

_, err = os.Stat(cfgPath)
assert.NoError(t, err)

cfg, err := ini.Load(cfgPath)
assert.NoError(t, err)

defaultSection, err := cfg.GetSection("DEFAULT")
assert.NoError(t, err)

assertKeyValueInSection(t, defaultSection, "host", "https://host")
assertKeyValueInSection(t, defaultSection, "token", "token")

// We should only save host and token for a profile, other env variables should not be saved
_, err = defaultSection.GetKey("auth_type")
assert.NotNil(t, err)
_, err = defaultSection.GetKey("metadata_service_url")
assert.NotNil(t, err)
}

func TestCustomProfileFromEnvConfigureNoInteractive(t *testing.T) {
ilia-db marked this conversation as resolved.
Show resolved Hide resolved
ctx := context.Background()
tempHomeDir := setup(t)
cfgPath := filepath.Join(tempHomeDir, ".databrickscfg")
Expand Down