Skip to content

Do not load host from bundle for CLI commands when profile flag is used #2335

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

Merged
andrewnester merged 5 commits into from
Feb 26, 2025
Merged

Do not load host from bundle for CLI commands when profile flag is used #2335

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
57c721e
Do not load host from bundle for CLI commands when profile flag is used
andrewnester Feb 11, 2025
a79504e
update test
andrewnester Feb 11, 2025
051d36e
make it local only
andrewnester Feb 12, 2025
f08770a
added comment
andrewnester Feb 25, 2025
df4e032
added bundle command tests
andrewnester Feb 25, 2025
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
4 changes: 2 additions & 2 deletions acceptance/auth/bundle_and_profile/output.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@
>>> errcode [CLI] current-user me -t dev -p DEFAULT
"[USERNAME]"

=== Inside the bundle, profile flag not matching bundle host. Badness: should use profile from flag instead and not fail
=== Inside the bundle, profile flag not matching bundle host. Should use profile from the flag and not the bundle.
>>> errcode [CLI] current-user me -p profile_name
Error: cannot resolve bundle auth configuration: config host mismatch: profile uses host https://non-existing-subdomain.databricks.com, but CLI configured to use [DATABRICKS_TARGET]
Error: Get "https://non-existing-subdomain.databricks.com/api/2.0/preview/scim/v2/Me": (redacted)

Exit code: 1

Expand Down
2 changes: 1 addition & 1 deletion acceptance/auth/bundle_and_profile/script
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ trace errcode $CLI current-user me -t dev | jq .userName
title "Inside the bundle, target and matching profile"
trace errcode $CLI current-user me -t dev -p DEFAULT | jq .userName

title "Inside the bundle, profile flag not matching bundle host. Badness: should use profile from flag instead and not fail"
title "Inside the bundle, profile flag not matching bundle host. Should use profile from the flag and not the bundle."
Copy link
Copy Markdown
Contributor

@denik denik Feb 25, 2025

Choose a reason for hiding this comment

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

nit: I'd replace "Should use profile from the flag" with 'Prefers profile passed via "-p profile_name" over the one specified by bundle host'.

trace errcode $CLI current-user me -p profile_name | jq .userName

title "Inside the bundle, target and not matching profile"
Expand Down
6 changes: 5 additions & 1 deletion acceptance/auth/bundle_and_profile/test.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
Badness = "When -p flag is used inside the bundle folder for any CLI commands, CLI use bundle host anyway instead of profile one"
LocalOnly=true

# Some of the clouds have DATABRICKS_HOST variable setup without https:// prefix
# In the result, output is replaced with DATABRICKS_URL variable instead of DATABRICKS_HOST
Expand All @@ -10,3 +10,7 @@ New='DATABRICKS_TARGET'
[[Repls]]
Old='DATABRICKS_URL'
New='DATABRICKS_TARGET'

[[Repls]]
Old='Get "https://non-existing-subdomain.databricks.com/api/2.0/preview/scim/v2/Me": .*'
New='Get "https://non-existing-subdomain.databricks.com/api/2.0/preview/scim/v2/Me": (redacted)'
6 changes: 6 additions & 0 deletions cmd/root/auth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,12 @@ func MustWorkspaceClient(cmd *cobra.Command, args []string) error {
cfg.Profile = profile
}

_, isTargetFlagSet := targetFlagValue(cmd)
if !isTargetFlagSet && hasProfileFlag {
Copy link
Copy Markdown
Contributor

@denik denik Feb 25, 2025

Choose a reason for hiding this comment

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

What happens if both -p and -t are set? Should that result in an error? or is it a valid use case as well?

Copy link
Copy Markdown
Contributor Author

@andrewnester andrewnester Feb 25, 2025

Choose a reason for hiding this comment

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

This is a valid use case, for example, you want to make a CLI call to -t dev but it requires some different profile so you need to provide -p flag. If the hosts mismatch in such cases it will correct fail

Copy link
Copy Markdown
Contributor

@denik denik Feb 25, 2025

Choose a reason for hiding this comment

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

If it's a valid case, should the condition be

if hasProfileFlag {?

so if -p is provided, then we always use that.

Copy link
Copy Markdown
Contributor Author

@andrewnester andrewnester Feb 25, 2025

Choose a reason for hiding this comment

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

if we use this condition we will skip bundle configuration always, even when -t is provided. But we don't want to skip it when -t is provided

// If the profile flag is set but the target flag is not, we should skip loading the bundle configuration.
cmd.SetContext(SkipLoadBundle(cmd.Context()))
Comment thread
shreyas-goenka marked this conversation as resolved.
Comment thread
andrewnester marked this conversation as resolved.
}

ctx := cmd.Context()
ctx = context.WithValue(ctx, &configUsed, cfg)
cmd.SetContext(ctx)
Expand Down
23 changes: 16 additions & 7 deletions cmd/root/bundle.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,26 +14,35 @@ import (

// getTarget returns the name of the target to operate in.
func getTarget(cmd *cobra.Command) (value string) {
target, isFlagSet := targetFlagValue(cmd)
if isFlagSet {
return target
}

// If it's not set, use the environment variable.
target, _ = env.Target(cmd.Context())
return target
}

func targetFlagValue(cmd *cobra.Command) (string, bool) {
Copy link
Copy Markdown
Contributor

@denik denik Feb 25, 2025

Choose a reason for hiding this comment

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

What's the motivation for ignoring env var but taking command line option into account?

Copy link
Copy Markdown
Contributor Author

@andrewnester andrewnester Feb 25, 2025

Choose a reason for hiding this comment

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

Do you mean not having the same behaviour like with flag set?
The motivation that is the usage intention is different. You're likely to set DATABRICKS_CONFIG_PROFILE env for all the commands you use, like databricks bundle deploy and databricks clusters list equally.
While providing a flag to command is an explicit way to want it to be executed against different profile.

// The command line flag takes precedence.
flag := cmd.Flag("target")
if flag != nil {
value = flag.Value.String()
value := flag.Value.String()
if value != "" {
return
return value, true
}
}

oldFlag := cmd.Flag("environment")
if oldFlag != nil {
value = oldFlag.Value.String()
value := oldFlag.Value.String()
if value != "" {
return
return value, true
}
}

// If it's not set, use the environment variable.
target, _ := env.Target(cmd.Context())
return target
return "", false
}

func getProfile(cmd *cobra.Command) (value string) {
Expand Down