Auth: resolve positional arg as profile name first#4840
Open
simonfaltum wants to merge 11 commits intomainfrom
Open
Auth: resolve positional arg as profile name first#4840simonfaltum wants to merge 11 commits intomainfrom
simonfaltum wants to merge 11 commits intomainfrom
Conversation
Co-authored-by: Isaac
Co-authored-by: Isaac
Co-authored-by: Isaac
Co-authored-by: Isaac
Co-authored-by: Isaac
Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Collaborator
|
Commit: 62ee5d5
19 interesting tests: 10 SKIP, 6 RECOVERED, 3 flaky
Top 20 slowest tests (at least 2 minutes):
|
- Fix token command skipping resolver when DATABRICKS_CONFIG_PROFILE is set by moving positional arg resolution before the env var read - Add test for login's --host + positional argument conflict guard - Align token command's Use string to PROFILE_OR_HOST for consistency - Add host:port detection (e.g., localhost:8080) to looksLikeHost - Improve resolveHostToProfile prompt label to "Select one to use" Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
requested review from
andrewnester,
anton-107,
denik,
pietern and
shreyas-goenka
as code owners
Suggested reviewersBased on git history of the changed files, these people are best suited to review:
Confidence: high Eligible reviewersBased on CODEOWNERS, these people or teams could also review: @andrewnester, @anton-107, @denik, @shreyas-goenka Suggestions based on git history of 8 changed files (6 scored). See CODEOWNERS for path-specific ownership rules. |
Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
removed request for
andrewnester,
anton-107,
denik and
shreyas-goenka
The positional argument is primarily a profile name. Host URL support is backwards compatibility, not the intended path forward, so we don't advertise it in --help output. Co-authored-by: Isaac
| if len(args) > 0 && authArguments.Host != "" { | ||
| return errors.New("please only provide a positional argument or --host, not both") | ||
| } | ||
| if profileName == "" && len(args) == 1 { |
Contributor
There was a problem hiding this comment.
Should we also error if there is a positional argument and the --profile flag?
|
|
||
| profileName := "" | ||
| profileFlag := cmd.Flag("profile") | ||
| if profileFlag != nil { |
Contributor
There was a problem hiding this comment.
Do we need this guard? IIUC, profile is a global flag and having it to nil would highlight a real bug that might be worth panicking over.
| arg string | ||
| wantProfile string | ||
| wantHost string | ||
| wantErr string |
Contributor
There was a problem hiding this comment.
Prefer using error sentinels (even private ones) that you can directly compare using errors.Is. This makes your test robust against change in error messages that still preserve the error semantic.
See example here: https://github.com/databricks/sdk-go/blob/main/core/profiles/profiles_test.go#L33
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Why
Running
databricks auth login logfoodtreatslogfoodas a host URL, which fails confusingly. Runningdatabricks auth token e2-logfood(a typo) falls through to host resolution, producing a misleading DNS error. The three auth commands handle positional arguments inconsistently: login only accepts hosts, token tries profile-first, logout tries profile-first.Changes
All three auth commands now share a
resolvePositionalArgfunction that resolves positional arguments as profile names first. If no profile matches and the argument doesn't look like a profile name, it returns a clear error.Before:
databricks auth login logfoodtries to resolvelogfoodas a hostname and fails.Now:
databricks auth login logfoodloads thelogfoodprofile and logs into its configured host.Before:
databricks auth token e2-logfoodproduces a confusing DNS/OAuth error.Now:
databricks auth token e2-logfoodproducesno profile named "e2-logfood" found.The usage strings show
[PROFILE]as the positional argument, reinforcing that profile is the primary concept. Host URLs still work as a silent fallback for backwards compatibility.Also removes the local
--profileflag fromauth logoutin favor of the global persistent flag, restoring-pshorthand consistency.Test plan
resolvePositionalArg(7 table-driven cases)resolveHostToProfile(4 cases)make checkspassesmake lintfullpassesgo test ./cmd/auth/passes