-
Notifications
You must be signed in to change notification settings - Fork 113
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
(#1268) Add mappings action to auth command #1285
Conversation
5a4d885
to
cd6abfe
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 other than that thing about the file, probably I wasn't clear about what I wanted :)
cli/auth_account_command.go
Outdated
mappingsaAdd.Arg("target", "The target subject of the mapping").StringVar(&c.mapTarget) | ||
mappingsaAdd.Arg("weight", "The weight (%) of the mappingmapping").Default("100").UintVar(&c.mapWeight) | ||
mappingsaAdd.Flag("operator", "Operator to act on").StringVar(&c.operatorName) | ||
mappingsaAdd.Flag("config", "JWT file to read configuration from").ExistingFileVar(&c.inputFile) |
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.
hmm, not sure we can really say read from a JWT, the idea with loading is that users can easily write a mapping in a file and load just that mapping, so we can kind of need to just load the jwt.Mapping
from a file in json/yaml format
So I dont think for that use case users really would have JWTs around, its a convenient way to avoid all the flags and stuff
cli/auth_account_command.go
Outdated
mappingsls.Flag("operator", "Operator to act on").StringVar(&c.operatorName) | ||
|
||
mappingsrm := mappings.Command("rm", "Remove a mapping").Action(c.mappingRmAction) | ||
mappingsrm.Arg("account", "Account to create the mappings on").StringVar(&c.accountName) |
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.
...remove the mappings from
74a01e0
to
9faa8af
Compare
Yeah woops, completely spoke past each other there. Updated to read the config from json/yaml |
mappings has `add`, `rm`, `list` and `info` commands. Add can take a --config flag pointing at a valid jwt, which it will then parse and extract the mappings from 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.
LGTM
mappings has
add
,rm
,list
andinfo
commands.Add can take a --config flag pointing at a valid jwt, which it will then parse and extract the mappings from it.