Eksctl: More consistent handling of invalid input and printing of usage/help

[marccarre]

What happened?

• For CLI options marked as required via cobra, the following happens when they are missing:

$ EKSCTL_EXPERIMENTAL=true ./eksctl enable repo
Error: required flag(s) "git-email", "git-url" not set
Usage: eksctl enable repo [flags]

Enable repository flags:
      --git-url string                    SSH URL of the Git repository to be used for GitOps, e.g. [email protected]:<github_org>/<repo_name>
      --git-branch string                 Git branch to be used for GitOps (default "master")
      --git-user string                   Username to use as Git committer (default "Flux")
      --git-email string                  Email to use as Git committer
      --git-private-ssh-key-path string   Optional path to the private SSH key to use with Git, e.g. ~/.ssh/id_rsa
      --git-paths strings                 Relative paths within the Git repo for Flux to locate Kubernetes manifests
      --git-label string                  Git label to keep track of Flux's sync progress; this is equivalent to overriding --git-sync-tag and --git-notes-ref in Flux (default "flux")
      --git-flux-subdir string            Directory within the Git repository where to commit the Flux manifests (default "flux/")
      --namespace string                  Cluster namespace where to install Flux, the Helm Operator and Tiller (default "flux")
      --with-helm                         Install the Helm Operator and Tiller (default true)
      --amend                             Stop to manually tweak the Flux manifests before pushing them to the Git repository

General flags:
      --cluster string       name of the EKS cluster to enable gitops on
  -r, --region string        AWS region
  -f, --config-file string   load configuration from a file (or stdin if set to '-')
      --timeout duration     maximum waiting time for any long-running operation (default 20s)

AWS client flags:
  -p, --profile string   AWS credentials profile to use (overrides the AWS_PROFILE environment variable)

Common flags:
  -C, --color string   toggle colorized logs (valid options: true, false, fabulous) (default "true")
  -h, --help           help for this command
  -v, --verbose int    set log level, use 0 to silence, 4 for debugging and 5 for debugging with AWS debug logging (default 3)

Use 'eksctl enable repo [command] --help' for more information about a command.


required flag(s) "git-email", "git-url" not set

• For input we merge from CLI and ClusterConfig and subsequently validate, no help is being printed:

$ EKSCTL_EXPERIMENTAL=true ./eksctl enable repo --git-email [email protected] --git-url https://foo.bar
[✖]  --cluster must be set

Note that I'd expect other commands have a similar behaviour.
We should fix them all to provide a better UX.

What you expected to happen?

1. Ideally, list all missing arguments.
2. Print the help every time.

[teaguecole]

Hey @marccarre i can work on this.

[teaguecole]

Hey @marccarre I looked in to this and you recently made a commit in which you added SilenceUsage = true to prevent it from being printed during genuine runtime errors. However, at the same time it is also is the reason for the printing inconsistency, I think? It might be worth having a chat about how you would want me to approach this. I am not sure how easy it would be to differentiate genuine runtime errors vs not using the CLI options correctly.

[marccarre]

@teaguecole, what I tried to convey in the above description is that we have two ways to validate input and therefore to report errors related to invalid input. This issue is unrelated to runtime errors.

1. One way is by marking flags as required in Cobra, which leads to errors like:
   Error: required flag(s) "git-email", "git-url" not set
2. Another way is using custom logic, typically put in ConfigLoaders like this one, which leads to errors like:
   [✖] --cluster must be set
   Context: ConfigLoaders were introduced for cases where we needed to aggregate input from the CLI and the ClusterConfig manifest file, and then validate the union of these.

Ideally, we should harmonise this and have only one way to report errors related to invalid input.

On top of this, in their current state, ConfigLoaders fail in a sequential way, meaning that they don't list all the missing or invalid input arguments, and then return that list to the end user, but instead fail on one, then when fixed fail onto the next one, etc.
For example, running this:

$ EKSCTL_EXPERIMENTAL=true ./eksctl enable repo --git-email [email protected] --git-url https://foo.bar
[✖]  --cluster must be set

fails to pass this validation logic:
https://github.com/weaveworks/eksctl/blob/43f9aa56ee7665fc3f2a96c5081242b54bcf6187/pkg/ctl/cmdutils/gitops.go#L117-L119
but if you then provide --cluster ${CLUSTER_NAME}:

$ EKSCTL_EXPERIMENTAL=true ./eksctl enable repo --git-email [email protected] --git-url https://foo.bar --cluster baz
[✖]  --region must be set

... you would still fail on:
https://github.com/weaveworks/eksctl/blob/43f9aa56ee7665fc3f2a96c5081242b54bcf6187/pkg/ctl/cmdutils/gitops.go#L120-L122

Ideally we should aggregate all these and report everything in one go, e.g.:

[✖]  --cluster must be set
[✖]  --region must be set

or

[✖]  --cluster and --region must be set

or so.
This would be less frustrating for the end-user who wouldn't have to do too many trials and errors to have eksctl achieve what they want.

I hope this makes sense & clarifies the issue. 🙂

[marccarre]

Once we have dealt with the above, I think we can decide whether to print or not the help, and do so in a consistent way too. (For now, though, we decided to temporarily disable its printing as simpler technically, and since this is how other commonly used CLIs behave, e.g. kubectl).

[teaguecole]

Thanks a lot! This definitely clears things up and points me in the right direction. I appreciate the time and effort you put in to that response. There are examples, code references, and everything! It's beautiful. I will take note of this if I ever need to clarify an issue for someone else.