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.

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

cli: nomad login command should not require a -type flag and should respect default auth method #16504

Merged
merged 10 commits into from
Mar 17, 2023

Conversation

pkazmierczak
Copy link
Contributor

@pkazmierczak pkazmierczak commented Mar 15, 2023

nomad login command does not need to know ACL Auth Method's type, since all method names are unique. I discovered this while fixing a bug in which login ignored the existence of default auth method and errored when users didn't provide a method name or type. I believe this bug comes from the fact that originally we intended to have default-per-type design, and later we decided it's better UX to have global-default auth methods.

Closes #16501.

@pkazmierczak pkazmierczak self-assigned this Mar 15, 2023
@pkazmierczak pkazmierczak added the backport/1.5.x backport to 1.5.x release line label Mar 15, 2023
@pkazmierczak pkazmierczak requested review from lgfa29 and jrasell March 15, 2023 13:27
@pkazmierczak pkazmierczak changed the title cli: fix a bug in the login command that ignores the default auth method cli: nomad login command should not require a -type flag and should respect default auth method Mar 15, 2023
Copy link
Member

@jrasell jrasell left a comment

Choose a reason for hiding this comment

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

LGTM, with a tiny grammar comment change. It might be worth pinging @mikenomitch to double check he is cool with the UX change.

It would also be worth preparing PR's for any tutorials that also need updating based on this, so they can be released at the right time.

.changelog/16504.txt Outdated Show resolved Hide resolved
command/login.go Outdated Show resolved Hide resolved
Co-authored-by: James Rasell <[email protected]>
Copy link
Contributor

@mikenomitch mikenomitch left a comment

Choose a reason for hiding this comment

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

+1 to the UX change from me

@pkazmierczak pkazmierczak merged commit b95b105 into main Mar 17, 2023
@pkazmierczak pkazmierczak deleted the b-login-default branch March 17, 2023 18:14
pkazmierczak added a commit that referenced this pull request Mar 17, 2023
…espect default auth method (#16504)

nomad login command does not need to know ACL Auth Method's type, since all
method names are unique. 

Co-authored-by: James Rasell <[email protected]>
pkazmierczak added a commit that referenced this pull request Mar 17, 2023
@tgross tgross added this to the 1.5.2 milestone Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.5.x backport to 1.5.x release line
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nomad login errors before determining the default auth method type
4 participants