Eksctl: zsh autocomplete error when calling eksctl create nodegroup - invalid option definition

[ysam12345]

What happened?
When I type eksctl create nodegroup [TAB]
It shows following error
_arguments:comparguments:319: invalid option definition: --version[Kubernetes version (valid options: 1.14, 1.15, 1.16, 1.17) [for nodegroups "auto" and "latest" can be used to automatically inherit version from the control plane or force latest]]:

Tested on my two macbook pro / Amazon Linux 2 EC2 Instance and saw the same error.

What you expected to happen?
Showing the autocomplete choices without any error.

How to reproduce it?

1. follow the guide to install eksctl autocompletion
   https://eksctl.io/introduction/#zsh
2. using zsh and type
   eksctl create nodegroup [TAB]

Anything else we need to know?
I think it's similar to this issue:
https://github.com/clap-rs/clap/issues/771
I can manually add escape character to ~/.zsh/completion/_eksctl line334

After that, it works fine

Versions

$ eksctl version
0.29.2
$ kubectl version
Client Version: version.Info{Major:"1", Minor:"17+", GitVersion:"v1.17.9-eks-4c6976", GitCommit:"4c6976793196d70bc5cd29d56ce5440c9473648e", GitTreeState:"clean", BuildDate:"2020-07-17T19:00:19Z", GoVersion:"go1.13.9", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"17+", GitVersion:"v1.17.9-eks-4c6976", GitCommit:"4c6976793196d70bc5cd29d56ce5440c9473648e", GitTreeState:"clean", BuildDate:"2020-07-17T18:46:04Z", GoVersion:"go1.13.9", Compiler:"gc", Platform:"linux/amd64"}

Logs

[ysam12345]

Created a docker image for testing this issue
DockerHub: https://hub.docker.com/repository/docker/ysam12345/test-eksctl-zsh-autocomplete

Script

docker run -ti --rm ysam12345/test-eksctl-zsh-autocomplete 
eksctl create nodegroup [TAB*2]

Output

[neha-viswanathan]

@michaelbeaumont @ysam12345 will the new release of spf13/cobra v1.10 help with this - https://github.com/spf13/cobra/pull/1070
They seem to have a newer zsh completion https://github.com/spf13/cobra/releases/tag/v1.1.0

[ysam12345]

@michaelbeaumont @neha-viswanathan Yes, I think this is work.
I changed the version of spf13/cobra from v1.0.0(Current) to v1.1.0 and rebuild eksctl.
The result looks good.

[michaelbeaumont]

@ysam12345 a PR would be very welcome!

[ysam12345]

Thanks @michaelbeaumont , I have created a pull request for this issue.

[ysam12345]

Hello @michaelbeaumont,
I closed pull request https://github.com/weaveworks/eksctl/pull/2756 because:

1. spf13/cobra release a new version v1.1.1 and I think we should upgrade to this version.
2. I have some concern when rewriting test case to fix the problem mentioned at https://github.com/weaveworks/eksctl/pull/2756#issuecomment-715898743.
   Currently I have make two version of code change to fix the test case but haven't create a pull request yet because I'm not sure which one is better solution.

A ysam12345:fix_zsh_completion_and_redirect_stdErr_to_stdOut

I changed string "out", returned by "execute" function in test, to contain error message from stdErr.
This does the smallest code change(17 lines) but is a little bit weird to forward stdErr to a string named "out".

B ysam12345:fix_zsh_completion_and_change_err_in_test

I changed error "err", returned by "execute" function in test, to have error message from stdErr when error is occur and rewrite some test case from checking message in "out" to checking message in "err"(L75-L80). I also rewrite some test condition from Equal to ContainSubstring , because the message from stdErr will be single/multiple lines with newline at the end. Also some test case L35-R38 will fail to pass lint if changed the string in fmt.Errorf to end with \n, so I can't use Equal condition except to avoid using fmt.Errorf.
Besides, I add "Error: " to test case string because the message from stdErr will be have "Error: " at the beginning of string.

I wonder which one is better idea? And any suggestion will be helpful.

Thanks!

[michaelbeaumont]

I think B is good. However, these tests need desperately to be refactored. They don't actually test the real behavior of the CLI since they mock so much, see e.g. the expected error output here:

https://github.com/weaveworks/eksctl/blob/7db9b8b7af3061361f247d7d15424ce70e1dc7e0/pkg/ctl/completion/completion_test.go#L39-L45

When I print the contents of out in the test I get Run 'completion --help' for usage.

With eksctl completion invalid-shell however I get as error Error: unknown resource type "invalid-shell" instead, I also get the usage itself printed out, not the message how to do it.

In reality, I don't see any difference in what's being printed where when I actually run eksctl, it seems to be only the tests

[ysam12345]

Yeah I see...... We won't see the content of out when we actually run the CLI with an invalid input, so the test is testing something user won't see.

I think maybe they should be rewrote to test for the actually output?

Please let me know if i understand wrong.

[michaelbeaumont]

Yes exactly, they need to be rewritten but not for your PR! :slightly_smiling_face:

[ysam12345]

Maybe I can help this in the future. 🙂
So I wonder is Plan B ok for a PR to fix this issue?

Thanks.

[michaelbeaumont]

@ysam12345 Yeah go ahead with plan b!