-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add -json
and -t
flag for nomad acl token create
command
#16055
Add -json
and -t
flag for nomad acl token create
command
#16055
Conversation
Signed-off-by: dttung2905 <[email protected]>
Signed-off-by: dttung2905 <[email protected]>
Signed-off-by: dttung2905 <[email protected]>
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.
Hi @dttung2905 and thanks for raising this; it's looking great. I've made a couple of very minor suggestions inline. I also wonder if it would be possible to update the tests added slightly, in particular when testing the JSON flag to check we have valid JSON returned in the output writer. Thanks again!
Signed-off-by: dttung2905 <[email protected]>
Hi @jrasell. Thanks for the super quick review. I didn't check the semantic of JSON output i.e. all KV pairs because I think its abit uncessary. Hence, I have only added a check for a proper JSON formatted output. Please let me know what you think on this 🙏 |
Signed-off-by: dttung2905 <[email protected]>
Signed-off-by: dttung2905 <[email protected]>
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.
Thanks for the updates @dttung2905 and LGTM! The unmarshal testing trick is something I have used elsewhere, so it's nice to see others see this as an approach to take. Thanks!
Hi team,
This PR aims to add
-json
and-t
flag fornomad acl token create
command. This is related to issue #15894Example output:
-json
flag-t
flag