-
Notifications
You must be signed in to change notification settings - Fork 0
Beta feedback 1 - login command ergonomics #32
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.
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
Beta feedback 1 - login command ergonomics #32
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to have just changed the way in which login isn't behaving as I'd expect, and even less intuitively than before - seems to me like the fix is in the wrong part of the issue.
First, a bit of preamble / user story to try and clarify:
As a user of the (planet) CLI, I expect options set using the CLI to apply to other commands made with the CLI.
Previously, the issue was I would do a planet profile set
, and then login as _PL_API_KEY, because the environment variable was set.
Now, it seems the result planet profile set
is also still ignored, and we're just falling back to the global default instead, which is still not what I'd expect.
(pl-sdk-v3) torben.barsballe@torben ~ % planet auth profile set pge
(pl-sdk-v3) torben.barsballe@torben ~ % planet auth profile show
Current: _PL_API_KEY
User Default: pge
Global Built-in Default: planet-user
(pl-sdk-v3) torben.barsballe@torben ~ % planet auth login --no-open-browser
Logging in with authentication profile planet-user...
Login succeeded.
Current value and user default differ.
Do you want to change the user default PL_AUTH_PROFILE from "pge" to "planet-user"? [y/N]: N
Warning: Environment variable "PL_API_KEY" is set. This will always take priority over the saved settings.
I do like the Warning: Environment variable "PL_API_KEY" is set. This will always take priority over the saved settings.
though, that is helpful. Looks like that came from #36 though, so I'd just say don't merge this and keep the current behaviour - I think that warning is the more important part.
I revised the example in my comment slightly as I'd misremembered the old default behaviour - conclusions are the same though. And I've now also tested this with the pure |
Closing this MR per today's discussion. Thanks for your time, @tbarsballe ! |
Change the root and auth method specific "login" commands to ignore of the automatic configuration from the environment. If a user is invoking login, they mean it.