feat(cli): Add ID option to 'proj role create-token' (#4632)

[tetchel]

Also add some more informative output to the same command

Signed-off-by: Tim Etchells tetchell@redhat.com

Checklist:

• Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
• The title of the PR states what changed and the related issues number (used for the release note).
• I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
• Does this PR require documentation updates?
• I've updated documentation as required by this PR.
• Optional. My organization is added to USERS.md.
• I've signed the CLA and my build is green (troubleshooting builds).

Fixes #4632

New help:

Flags:
  -e, --expires-in string   Duration before the token will expire, eg "12h", "7d". (Default: No expiration)
  -h, --help                help for create-token
  -i, --id string           Token unique identifier. (Default: Random UUID)

New output:

[ /argo-cd/cmd/argocd ] 29 (master) $ ./argocd proj role create-token default tim --id tim11
Created token succeeded.
  Project: default
  Role: tim
  ID: tim11
  Expires At: Never
  Token: eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJqdGkiOiJ0aW0xMSIsImlhdCI6MTYwMzM0MDM5NCwiaXNzIjoiYXJnb2NkIiwibmJmIjoxNjAzMzQwMzk0LCJzdWIiOiJwcm9qOmRlZmF1bHQ6dGltIn0.svZ7lxBfSNnZ6FHBlT-G9mjS0OJTkvPWFzdM4t0gK-o

If the server does not provide ID in the response (the behaviour before this PR), then just Project, Role, and the token are output; this is intended to allow compatibility with older versions of the argoCD server.

[codecov-io]

Codecov Report

Merging #4636 into master will decrease coverage by 0.08%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #4636      +/-   ##
==========================================
- Coverage   41.31%   41.23%   -0.09%     
==========================================
  Files         124      124              
  Lines       16976    17013      +37     
==========================================
+ Hits         7014     7015       +1     
- Misses       8950     8987      +37     
+ Partials     1012     1011       -1     

Impacted Files	Coverage Δ
cmd/argocd/commands/project_role.go	0.00% <0.00%> (ø)
util/settings/settings.go	40.06% <0.00%> (-0.49%)	⬇️
controller/appcontroller.go	52.59% <0.00%> (-0.06%)	⬇️
pkg/apis/application/v1alpha1/types.go	57.70% <0.00%> (ø)
reposerver/repository/repository.go	60.61% <0.00%> (+0.61%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7824a1f...ffb42bc. Read the comment docs.

[jessesuen]

@tetchel FYI, ID and expiresAt are already encoded in the token JWT. Do you think it's too inconvenient to decode it from the token or do you feel like it needs to be surfaced in the response?

[tetchel]

It does not look like the Expires At is encoded but I suppose I could just leave that out of this output and decode the ID on the client side. then, no api server changes would be required.

[tetchel]

would you like me to make that change?

[jannfis]

The expiry time is only encoded if explicitly set - i.e. a token created with --expires-in option.

[tetchel]

that make sense, a null expiry means it never expires.

OK, doing the decode on the client-side makes sense then.

[jannfis]

Just a thought about changing the output of the command - I believe that project token creation could be used in some automation scripts, and this change would break those scripts.

While I like that it prints detailed information about the created token, I'm a little unsure whether we should go ahead and change the output by default or if it's better to "hide" it behind a --verbose|-v switch. Or, when we choose to make this the default, we should have an option to just print the plain token output to stdout to restore previous behaviour, and have it noted in the upgrade/release notes so people have a chance to change their scripts before upgrading.

[tetchel]

Flagging it is a good idea, you're right about breaking backwards compatibility for scripts.

I think the new output is better as the default behaviour, but I agree that it will cause pain for some users.

So either:

1. The default output is only the token and provide --verbose for the compete output.
   Pro: Doesn't break existing scripts.
   Con: The complete output will be harder to discover, and used less often.
2. The default output is the complete output and the token only is hidden behind some other flag, --token-only|t or --simple|s or something like that.
   Pro: Complete output by default
   Con: Requires a release note that scripts will break and users will have to adapt.

Let me know which option you prefer.

[jannfis]

This is a rather tough one :) Usability vs. backwards-compatibility. I know that some people actually are using means like curl or wget to the argocd-server to get its version of the CLI, and then use it with their scripts. So they expect a fair amount of backwards compatibility, and they might not be the same people that are responsible for the life-cycle of the Argo CD installation, just plain consumers. They might have not read the release notes.

BUT. There's a big BUT. I think this is not good practice to auto-update CLI version for scripting, and I also think this is a very small edge case with a very quick fix (i.e. adapt script to use CLI with new cmdline parameter), instead of re-writing logic (i.e. parse new output).

So I'd be fine going with option 2) and having a new switch --token-only|t to only dump the raw base64 encoded token to stdout.

Other voices on this?

[tetchel]

updated:

[ /argo-cd/cmd/argocd ] 23 (customTokenID) $ go build . && ./argocd proj role create-token default tim --id hello --expires-in 10m
Create token succeeded for proj:default:tim.
  ID: hello
  Issued At: 2020-10-24T19:23:49-04:00
  Expires At: 2020-10-24T19:33:49-04:00
  Token: eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJleHAiOjE2MDM1ODI0MjksImp0aSI6ImhlbGxvIiwiaWF0IjoxNjAzNTgxODI5LCJpc3MiOiJhcmdvY2QiLCJuYmYiOjE2MDM1ODE4MjksInN1YiI6InByb2o6ZGVmYXVsdDp0aW0ifQ.u-Pd3ecbJiQeL5qAvJb5YTQMQ11ELkw0QQzjGqd35kc
[ /argo-cd/cmd/argocd ] 24 (customTokenID) $ go build . && ./argocd proj role create-token default tim --id hello2 --expires-in 10m --token-only
eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJleHAiOjE2MDM1ODI0NTMsImp0aSI6ImhlbGxvMiIsImlhdCI6MTYwMzU4MTg1MywiaXNzIjoiYXJnb2NkIiwibmJmIjoxNjAzNTgxODUzLCJzdWIiOiJwcm9qOmRlZmF1bHQ6dGltIn0.rLWYNheu4zcIyQFmoDSMIVoUlSKFoiJ8I_D7qctKFTA

[jannfis]

LGTM!

I marked this PR as low-impact possibly breaking change, so that we can catch up in the release notes.

[jannfis]

@jessesuen / @alexmt FYI