feat(cli): dynamic shell completion for main resources names (rollouts, experiments, analysisrun)

[thomas-riccardi]

MVP for #2015

Add dynamic shell completion for all commands that take a ROLLOUT_NAME, EXPERIMENT_NAME, ANALYSISRUN_NAME argument.

Implementation details

Inspired from what was done in kubectl when they migrated to go completion kubernetes/kubernetes#96087, as well as a later refactor (kubernetes/kubernetes@c4f8c57).

• use ValidArgsFunction from Cobra to list candidates for completion: https://github.com/spf13/cobra/blob/main/shell_completions.md#dynamic-completion-of-nouns
• use v2 of GenBashCompletion from cobra: https://github.com/spf13/cobra/blob/main/shell_completions.md#bash-completion-v2
  • I chose to disable descriptions for completion (as a bash user I'm not used to that), but it's enabled for fish it seems, we can enable it if desired, I have no strong opinion on it
• all factorized in new package pkg/kubectl-argo-rollouts/util/completion, with its own tests
  • one function per resource type; I could not find an easy way to factorize all types (I'm new in golang)
  • tests reuse info/testdata but we could build our own data for more isolated tests if needed
• implemented completion and used them everywhere
  • by grepping into the auto-generated CLI doc to find all the usages (so maybe I missed some if the Use strings are not all correct)
  • for 3 types: Rollout, Experiment, AnalysisRun
    • missing 2 types related to notifications, but for that I need to patch in another git repo: https://github.com/argoproj/notifications-engine/blob/master/pkg/cmd/trigger.go => this will be for another PR
• it seems the cli Use strings are not showing when the command supports multiple resources, at least for abort: for now only the first resource name is completed; I could support more but we should probably start by documenting those cases in the Use and maybe Short?
• no bash default completion on file disabled on the other commands (cf kubectl-argo-rollouts __complete lint "": Completion ended with directive: ShellCompDirectiveDefault): I don't know if we can disable it by default or if we need to add it everywhere.
• no dynamic completion on flags for now, such as create -f or lint -f
• no dynamic completion on set image second arg: CONTAINER=IMAGE for now

Checklist:

• Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
• The title of the PR is (a) conventional with a list of types and scopes found here, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
• I've signed my commits with DCO
• I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
• My builds are green. Try syncing with master if they are not.
• My organization is added to USERS.md.

[github-actions]

Go Published Test Results

1 784 tests   1 784 ✔️  2m 30s ⏱️
   102 suites         0 💤
       1 files           0 ❌

Results for commit 17851c8.

♻️ This comment has been updated with latest results.

[github-actions]

E2E Tests Published Test Results

    2 files      2 suites   1h 33m 31s ⏱️
  89 tests   82 ✔️ 3 💤 4 ❌
184 runs  172 ✔️ 6 💤 6 ❌

For more details on these failures, see this check.

Results for commit 17851c8.

♻️ This comment has been updated with latest results.

[zachaller]

filtering on 'toComplete' doesn't seem necessary, at least on bash (most probably bash-completion handles that itself), not sure for the others; I could not find cobra documentation about that

I am maybe missing what exactly this would get us. I am a fish user and things seem to also be working as I would expect with the first completion you added in your WIP PR so I think it would be ok to not do this as well.

I chose to disable descriptions for completion (as a bash user I'm not used to that), but it's enabled for fish it seems, we can enable it if desired, I have no strong opinion on it

Yes for fish this make sense to leave on and I think it is generally expected and nice as far as bash I am also impartial as a non bash user, I am fine leaving it to false and seeing if a request comes in later to change etc.

Regarding unit-tests, should I go the way it was done in kubectl when they migrated to go completion kubernetes/kubernetes#96087? (they were able to factorize most ValidArgsFunctions and do some test on those, but not on custom ones it seems).
Are such unit-tests blocking for merging this PR?

I took a quick look at the kubectl testing and this would also work well here. With some of the stances within ArgoProj coming up around PR acceptance criteria having unit tests would be ideal as well.

Also thank you for contributing and working on this feature it will be really nice to have!

[thomas-riccardi]

filtering on 'toComplete' doesn't seem necessary, at least on bash (most probably bash-completion handles that itself), not sure for the others; I could not find cobra documentation about that

I am maybe missing what exactly this would get us. I am a fish user and things seem to also be working as I would expect with the first completion you added in your WIP PR so I think it would be ok to not do this as well.

The cobra spec I could find is unclear: maybe it's mandatory we do the filtering in go. But it doesn't seem to be the case, thanks to your fish test.
=> good for me

In fact, after reading more in the kubectl PR, I did implement the filter in go.

I chose to disable descriptions for completion (as a bash user I'm not used to that), but it's enabled for fish it seems, we can enable it if desired, I have no strong opinion on it

Yes for fish this make sense to leave on and I think it is generally expected and nice as far as bash I am also impartial as a non bash user, I am fine leaving it to false and seeing if a request comes in later to change etc.

=> good for me

Regarding unit-tests, should I go the way it was done in kubectl when they migrated to go completion kubernetes/kubernetes#96087? (they were able to factorize most ValidArgsFunctions and do some test on those, but not on custom ones it seems).
Are such unit-tests blocking for merging this PR?

I took a quick look at the kubectl testing and this would also work well here. With some of the stances within ArgoProj coming up around PR acceptance criteria having unit tests would be ideal as well.

It will take me more time to complete that, I'm not sure I can finish before the end of hacktoberfest.
For a first PR, would you prefer a dynamic completion for a restricted subset of the commands, with unit tests, or more commands but no unit-tests? Or neither are acceptable for merge?

[codecov]

Codecov Report

Base: 82.76% // Head: 82.76% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (17851c8) compared to base (381feea).
Patch coverage: 83.63% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2379   +/-   ##
=======================================
  Coverage   82.76%   82.76%           
=======================================
  Files         121      122    +1     
  Lines       18536    18590   +54     
=======================================
+ Hits        15341    15386   +45     
- Misses       2410     2416    +6     
- Partials      785      788    +3     

Impacted Files	Coverage Δ
pkg/kubectl-argo-rollouts/cmd/abort/abort.go	100.00% <ø> (ø)
...kg/kubectl-argo-rollouts/cmd/get/get_experiment.go	65.26% <ø> (ø)
pkg/kubectl-argo-rollouts/cmd/get/get_rollout.go	98.55% <ø> (ø)
pkg/kubectl-argo-rollouts/cmd/pause/pause.go	100.00% <ø> (ø)
pkg/kubectl-argo-rollouts/cmd/promote/promote.go	96.33% <ø> (ø)
pkg/kubectl-argo-rollouts/cmd/restart/restart.go	90.00% <ø> (ø)
pkg/kubectl-argo-rollouts/cmd/retry/retry.go	100.00% <ø> (ø)
pkg/kubectl-argo-rollouts/cmd/set/set_image.go	82.35% <ø> (ø)
pkg/kubectl-argo-rollouts/cmd/status/status.go	94.18% <ø> (ø)
...g/kubectl-argo-rollouts/cmd/terminate/terminate.go	100.00% <ø> (ø)
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[thomas-riccardi]

Current state:

• factorized the completion functions
  • in new util hierarchy: pkg/kubectl-argo-rollouts/util/completion/completion.go, following what was done in kubectl; let me know if I should move that elsewere
  • per resource type; I could not find an easy way to factorize all types (I'm new in golang)
• implemented completion and used them everywhere
  • by grepping into the auto-generated CLI doc to find all the usages (so maybe I missed some if the Use strings are not all correct)
  • for 3 types: Rollout, Experiment, AnalysisRun
    • missing 2 types related to notifications, but for that I need to patch in another git repo: https://github.com/argoproj/notifications-engine/blob/master/pkg/cmd/trigger.go => this will be for another PR
• no unit-test for now, although with the partial factorization it should be tractable
• it seems the cli Use strings are not showing when the command supports multiple resources, at least for abort: for now only the first resource name is completed; I could support more but we should probably start by documenting those cases in the Use and maybe Short?
• no bash default completion on file disabled on the other commands (cf kubectl-argo-rollouts __complete lint "": Completion ended with directive: ShellCompDirectiveDefault): I haven't yet read the doc/usages to know if we can disable it by default or if we need to add it everywhere.
• no dynamic completion on flags for now, such as create -f or lint -f
• no dynamic completion on set image second arg: CONTAINER=IMAGE for now

[zachaller]

I think doing a dynamic completion for a restricted subset of the commands with tests would be prefered for hacktoberfest. Even if you don't finish the PR 100% completely but it is close I don't mind adding the label to it if you need a bit more time to finish up after as well.

in new util hierarchy: pkg/kubectl-argo-rollouts/util/completion/completion.go, following what was done in kubectl; let me know if I should move that elsewere

I think this is a good location for this package

[thomas-riccardi]

I think doing a dynamic completion for a restricted subset of the commands with tests would be prefered for hacktoberfest. Even if you don't finish the PR 100% completely but it is close I don't mind adding the label to it if you need a bit more time to finish up after as well.

I started looking into the tests (I'm thinking about reusing pkg/kubectl-argo-rollouts/info/testdata), but I'm not sure I'll have a ready PR before end of month. I'd like the label please :)

[thomas-riccardi]

I've pushed unit tests, directly in the new completion pacakge, like it was done in kubectl (kubernetes/kubernetes#96087, as well as a later refactor (kubernetes/kubernetes@c4f8c57)).

• fake cmd to avoid circular import
• reuse info/testdata but we could build our own data for more isolated tests if needed
• I don't really understand what I'm doing with all the initializations in the tests (mainly tf): it's a mix of what I found in argo-rollouts and kubectl tests , but it seems to work...

[thomas-riccardi]

I've completed the PR description: the PR is ready for review from my PoV.

[zachaller]

Could you please try to just trigger tests again with a command something like git commit -s -S --allow-empty -m "github trigger re-run" or just push some commit I am not sure why e2e tests where failing want to make sure its a true failure and not just a github issue etc etc.

[zachaller]

I actually think the e2e errors are because of not pinning a k8s version in e2e tests, I will work on getting that fixed.

[thomas-riccardi]

I actually think the e2e errors are because of not pinning a k8s version in e2e tests, I will work on getting that fixed.

TestFunctionalSuite/TestRolloutPDBRestart also failed in my other PR #2375

when working on the multi k8s version e2e tests PR (#2380) I also have some tests failing, but different ones, and only on k8s 1.23 (not sure if those are hard or flaky tests..)

[zachaller]

There are quite a bit of flaky tests but they do retry, I got the multi k8s versions PR merged can you trigger a retest on this PR

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
6 Code Smells

No Coverage information
0.0% Duplication