-
Notifications
You must be signed in to change notification settings - Fork 72
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
feat: add token command #1517
feat: add token command #1517
Conversation
pkg/cmd/token/token.go
Outdated
} | ||
|
||
cmd := &cobra.Command{ | ||
Use: "token", |
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.
@rkpattnaik780 once we merge this we have whoami and token commands now.
Maybe it is worth to introduce auth and keep two commands there?
I'm worried about too many commands in root of cli.
Otherwise we can do
Use: "token", | |
Use: "auth-token", |
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.
Alternatively we can put token as hidden command
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.
@wtrocki I updated cmd name as auth-token but build is failing due to test failing, seems validator_test
is failing.
If I'm not wrong validator_test.go rules doesn't allow to use - dash
in cmd name.
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.
That was actually bad advice on my side. I will wait for @rkpattnaik780 opinion about the command name and we can merge it.
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.
whoami
command looks good where it is right now, I dont feel good about introducing "auth".
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.
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.
Command name is not going to pass validation. @rkpattnaik780 do you mind adjusting name to authtoken or maybe we can have separate group for whoami and token?
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.
do you mind adjusting name to authtoken or maybe we can have separate group for whoami and token?
authtoken seems a better name. I strongly feel whoami should be at root.
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.
World class work! Thank you so much.
FYI token we return is control plane token. |
4f07751
to
c8aff19
Compare
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 great! :D
Changing name in separate PR. All changes will be merged |
Closes # https://issues.redhat.com/browse/MGDSTRM-8103
Verification Steps
Run the following command
Type of change
Checklist