-
Notifications
You must be signed in to change notification settings - Fork 395
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
Auth "init", "switch", "remove" Forces Lowercase #1362
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.
Looks good so far.
Since the context flag can be included with any command (doesn't switch the context in the config), we'll also need to update the getContextAccessToken
func in:
https://github.com/digitalocean/doctl/blob/main/commands/command_config.go#L136
It would also be good to consider tests to cover these cases.
Thanks for the feedback. I was going back and forth on whether to make the "getContext" case insensitive. But I agree to it, to truly make it a case insensitive experience throughout. The tests were a little tricky. You can't pass in the "context" arg like you would any other argument in doctl. So I tested what I could. |
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 didn't work for me when using the flag (i.e. go run ./cmd/doctl --context TEST compute d ls).
I left a different suggestion that resolves it inline.
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.
LGTM 👍🏽
Background
We've had several requests report that they are seeing a discrepancy in doctl authentication. When they create a context with an uppercase letter, the context name is stored in all lowercase and causes the following:
Currently, this is what happens:
In this code snippet, you can see that the context is created as Chandan but it is stored as chandan. When the context Chandan is used you can see it is accepted but the authentication fails. But when the context is used as chandan authentication works.
Ideally, when the uppercase context is used it should throw an error. This is an issue with Viper, a third party package doctl uses. This seems to be the most requested feature that the tool has yet to be supported:
spf13/viper#1014
Solution
Though not ideal, I'm presenting a rather simple work around of lower-casing the
context
behind the scenes for the user and disclosing that the context names are case insensitive in the doctl man pages. With these changes, this is what would happen in the above scenario instead:As you can see, the context is case insensitive now and can switch between existing contexts smoothly. It will also error when the context does not exist.
Considerations
Unless I'm missing a specific case scenario, I do not believe this introduces a breaking change as all previously created contexts have been stored in all lowercase.
APICLI-1828
Addresses: #411