Auth: resolve positional arg as profile name first#4840
Open
simonfaltum wants to merge 17 commits intomainfrom
Open
Auth: resolve positional arg as profile name first#4840simonfaltum wants to merge 17 commits intomainfrom
simonfaltum wants to merge 17 commits intomainfrom
Conversation
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):
|
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. |
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
simonfaltum
requested review from
renaudhartert-db
and removed request for
pietern
Co-authored-by: Isaac
Co-authored-by: Isaac
Co-authored-by: Isaac
Co-authored-by: Isaac
Co-authored-by: Isaac
Co-authored-by: Isaac
- 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
Co-authored-by: Isaac
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
…arg conflict - Use errNoProfileFound sentinel error instead of formatted string for resolvePositionalArg, enabling errors.Is checks in tests - Remove unnecessary nil guard on global --profile flag in logout and token commands (profile is always registered as a persistent root flag) - Add positional arg + --profile conflict check in login command Co-authored-by: Isaac
…lict The testifylint linter requires assert.ErrorIs over assert.True(errors.Is). The blanket rejection of positional arg + --profile in login was wrong: `databricks auth login https://host --profile myprofile` is valid (host as positional arg with explicit profile). The profile-first resolution already skips when profileName is set, so no extra guard is needed. Co-authored-by: Isaac
The positional argument is a shorthand that resolves to either a profile or a host. Combining it with explicit flags is ambiguous, so we now error with a user-friendly message that echoes the argument and suggests using the flags directly. This is consistent across login, logout, and token. Co-authored-by: Isaac
Co-authored-by: Isaac
We silently support host URLs as positional args but don't want to advertise it in the usage line. Co-authored-by: Isaac
simonfaltum
force-pushed
the
simonfaltum/auth-positional-profile
branch
from
fdfea5a to
f4bf117
Compare
The profileHostConflictCheck PreRunE hook now rejects login when --profile and --host specify different hosts. Update the test to expect the conflict error instead of a successful override. Co-authored-by: Isaac
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