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

Jump to bottom

Use profile information when getting a token using the CLI #855

Merged
merged 3 commits into from
Oct 11, 2023
Merged

Use profile information when getting a token using the CLI #855

merged 3 commits into from
Oct 11, 2023

Conversation

hectorcast-db
Copy link
Contributor

@hectorcast-db hectorcast-db commented Oct 10, 2023 •
edited
Loading

Changes

Use stored profile information when the user provides the profile flag when using the databricks auth token command.

Tests

Run the command with and without the profile flag

./cli auth token
Databricks Host: https://e2-dogfood.staging.cloud.databricks.com/
{
  "access_token": "****",
  "token_type": "Bearer",
  "expiry": "2023-10-10T14:24:11.85617+02:00"
}%

./cli auth token --profile DEFAULT
{
  "access_token": "*****",
  "token_type": "Bearer",
  "expiry": "2023-10-10T14:24:11.85617+02:00"
}%

./cli auth token https://e2-dogfood.staging.cloud.databricks.com/
{
  "access_token": "*****",
  "token_type": "Bearer",
  "expiry": "2023-10-11T09:24:55.046029+02:00"
}%   

./cli auth token --profile DEFAULT https://e2-dogfood.staging.cloud.databricks.com/
Error: providing both a profile and a host parameters is not supported

@andrewnester
Copy link
Contributor

Fixes #746

andrewnester
andrewnester reviewed Oct 10, 2023
View reviewed changes
cmd/auth/token.go
@@ -21,9 +22,27 @@ func newTokenCommand(persistentAuth *auth.PersistentAuth) *cobra.Command {

cmd.RunE = func(cmd *cobra.Command, args []string) error {
ctx := cmd.Context()

var profileName string
Copy link
Contributor

Choose a reason for hiding this comment

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

What would happen if we run the command with host positional argument and profile flag? Like

databricks auth token https://foo.com --profile bar

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be ignored. The profile flag is used only to get the host from the config. I can't see any valid use-case of passing both as parameters.

Copy link
Contributor

Choose a reason for hiding this comment

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

The confusing point here is when there's a mismatch between profile host and host in argument. We either should not allow to provide both at the same time or error out if there's host mismatch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we throw an error if the url is provided on top of the profile?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say we throw an error when host from profile and host from args do not match. @pietern wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the value of doing that over simply erroring because both a profile name and a host are specified.

Are there valid cases where you specify both?

If not we should make these mutually exclusive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only use the profile to select the host. So currently there is no use case for passing both. I added an error if they are both specified.

cmd/auth/token.go Show resolved Hide resolved
@hectorcast-db hectorcast-db added this pull request to the merge queue Oct 11, 2023
Merged via the queue into main with commit 77101c9 Oct 11, 2023
@hectorcast-db hectorcast-db deleted the token-profile-flag branch October 11, 2023 11:17
andrewnester added a commit that referenced this pull request Oct 11, 2023
@andrewnester
Release v0.207.1
8024968 
CLI:
 * Improve `workspace import` command by allowing references to local files for content ([#793](#793)).
 * Add `--file` flag to workspace export command ([#794](#794)).
 * Ensure profile flag is respected for sync command ([#837](#837)).
 * Add hint to delete sync snapshot if parsing fails ([#853](#853)).
 * Use profile information when getting a token using the CLI ([#855](#855)).

Bundles:
 * Minor template tweaks ([#832](#832)).
 * Fixed using repo files as pipeline libraries ([#847](#847)).
 * Support .gitignore syntax in sync section and make sure it works recursively ([#854](#854)).
 * Allow target overrides for sync section ([#856](#856)).

Internal:
 * Fix import export integration tests on windows ([#842](#842)).
 * Fix workspace import test ([#844](#844)).
 * Automatically create a release PR in homebrew-tap repo ([#841](#841)).

Dependency updates:
 * Bump golang.org/x/term from 0.12.0 to 0.13.0 ([#852](#852)).
 * Bump golang.org/x/mod from 0.12.0 to 0.13.0 ([#851](#851)).
 * Bump golang.org/x/sync from 0.3.0 to 0.4.0 ([#849](#849)).
 * Bump golang.org/x/oauth2 from 0.12.0 to 0.13.0 ([#850](#850)).
@andrewnester andrewnester mentioned this pull request Oct 11, 2023
Merged
github-merge-queue bot pushed a commit that referenced this pull request Oct 11, 2023
@andrewnester
Release v0.207.1 (#860)
420a01e 
CLI:
* Improve `workspace import` command by allowing references to local
files for content ([#793](#793)).
* Add `--file` flag to workspace export command
([#794](#794)).
* Ensure profile flag is respected for sync command
([#837](#837)).
* Add hint to delete sync snapshot if parsing fails
([#853](#853)).
* Use profile information when getting a token using the CLI
([#855](#855)).

Bundles:
* Minor template tweaks
([#832](#832)).
* Fixed using repo files as pipeline libraries
([#847](#847)).
* Support .gitignore syntax in sync section and make sure it works
recursively ([#854](#854)).
* Allow target overrides for sync section
([#856](#856)).

Internal:
* Fix import export integration tests on windows
([#842](#842)).
* Fix workspace import test
([#844](#844)).
* Automatically create a release PR in homebrew-tap repo
([#841](#841)).


Dependency updates:
* Bump golang.org/x/term from 0.12.0 to 0.13.0
([#852](#852)).
* Bump golang.org/x/mod from 0.12.0 to 0.13.0
([#851](#851)).
* Bump golang.org/x/sync from 0.3.0 to 0.4.0
([#849](#849)).
* Bump golang.org/x/oauth2 from 0.12.0 to 0.13.0
([#850](#850)).
hectorcast-db added a commit that referenced this pull request Oct 13, 2023
@hectorcast-db
Use profile information when getting a token using the CLI (#855)
57b5f2c 
## Changes
Use stored profile information when the user provides the profile flag
when using the `databricks auth token` command.

## Tests
Run the command with and without the profile flag

```
./cli auth token
Databricks Host: https://e2-dogfood.staging.cloud.databricks.com/
{
  "access_token": "****",
  "token_type": "Bearer",
  "expiry": "2023-10-10T14:24:11.85617+02:00"
}%

./cli auth token --profile DEFAULT
{
  "access_token": "*****",
  "token_type": "Bearer",
  "expiry": "2023-10-10T14:24:11.85617+02:00"
}%

./cli auth token https://e2-dogfood.staging.cloud.databricks.com/
{
  "access_token": "*****",
  "token_type": "Bearer",
  "expiry": "2023-10-11T09:24:55.046029+02:00"
}%   

./cli auth token --profile DEFAULT https://e2-dogfood.staging.cloud.databricks.com/
Error: providing both a profile and a host parameters is not supported
```
hectorcast-db pushed a commit that referenced this pull request Oct 13, 2023
@andrewnester @hectorcast-db
Release v0.207.1 (#860)
c35b207 
CLI:
* Improve `workspace import` command by allowing references to local
files for content ([#793](#793)).
* Add `--file` flag to workspace export command
([#794](#794)).
* Ensure profile flag is respected for sync command
([#837](#837)).
* Add hint to delete sync snapshot if parsing fails
([#853](#853)).
* Use profile information when getting a token using the CLI
([#855](#855)).

Bundles:
* Minor template tweaks
([#832](#832)).
* Fixed using repo files as pipeline libraries
([#847](#847)).
* Support .gitignore syntax in sync section and make sure it works
recursively ([#854](#854)).
* Allow target overrides for sync section
([#856](#856)).

Internal:
* Fix import export integration tests on windows
([#842](#842)).
* Fix workspace import test
([#844](#844)).
* Automatically create a release PR in homebrew-tap repo
([#841](#841)).


Dependency updates:
* Bump golang.org/x/term from 0.12.0 to 0.13.0
([#852](#852)).
* Bump golang.org/x/mod from 0.12.0 to 0.13.0
([#851](#851)).
* Bump golang.org/x/sync from 0.3.0 to 0.4.0
([#849](#849)).
* Bump golang.org/x/oauth2 from 0.12.0 to 0.13.0
([#850](#850)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pietern pietern pietern left review comments

@andrewnester andrewnester andrewnester approved these changes

@mgyucht mgyucht Awaiting requested review from mgyucht

@tanmay-db tanmay-db Awaiting requested review from tanmay-db

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hectorcast-db @andrewnester @pietern