RFE (small suggestion): Error faced when we run `kubectl preflight` does not clarifies what is missing and why

[camilamacedo86]

Describe the rationale for the suggested feature.

If we install troubleshoot and run kubectl preflight to check it is ok the following error is returned which can bring confusion and does not let us know what is properly missing and why, or if the tool is properly installed.

$ kubectl preflight Error: requires at least 1 arg(s), only received 0

Describe the feature

Why the URL is required as arg? Could I am as user use any other value that is not https://preflight.replicated.com?

What about simplify its usage by:

• Using flag instead of arg to pass the URL https://preflight.replicated.com
• If the URL not to be set, then its default value will be used which would be a constant with https://preflight.replicated.com

So that, I can run the command with kubectl preflight only.

Describe alternatives you've considered

Return a better message so that we can clarifies that it is missing a URL with its default value.

[diamonwiggins]

Thanks for raising this one @camilamacedo86 .

@xavpaice Thoughts on https://preflight.replicated.com being a default here? For the support bundle binary we don't have a default either we just throw https://github.com/replicatedhq/troubleshoot/blob/main/cmd/troubleshoot/cli/run.go#L42-L44 if you omit an argument and don't specify load-cluster-spec.

Whatever we decide here would be great to make the support bundle binary and preflight binaries consistent.

[chris-sanders]

Why the URL is required as arg? Could I am as user use any other value that is not https://preflight.replicated.com?

Yes you can, and that's precisely why it's not a default.

I'll reference the design principal https://github.com/replicatedhq/troubleshoot/blob/main/design/design-principles.md#tools-not-specs

This is why I don't think we should default a spec. Replicated might have a default spec we like, but Troubleshoot itself is intentionally not suppose to be the source of truth for specs. It's a set of tools, and the user needs to decide the spec. Based on that, I'm not in favor of making a default URL I think it should remain a user input.

[xavpaice]

I really don't think we can have a preflight default that adds value for an application, each one is going to have specific things it wants to check for. I do think the error message could be improved though.

[camilamacedo86]

Hi @xavpaice, @chris-sanders, @diamonwiggins,

Thank you for your reply and time.

I see, and all makes sense. I think the only improvement that we could have here are:

• a) We move it for flag and add a description to let people knows what URL should add there.

• b) (maybe) We be able to set the value also via ENV VAR so that if the flag is not populated we could use the the env var value. That would allow we set default "troubleshoot spec" for each env/use case instead of ask for an end-user knows exactly what value / url should use in the specific scenarios. WDYT? Would that make sense?

Also, would it need to be an URL/link, could not that be an yaml file path as well?

[chris-sanders]

Can you explain why you would prefer a mandatory user input be a flag instead of an argument?

Similarly, why would you choose this as an ENV specifically. Would you make all CLI flags available as ENV variables as well or only this one? I'm not aware of any standard pattern of CLI utilities using ENV variables for inputs unless we're trying to make it easy to run in a container at which point I think addressing that mapping in the container definition would absolutely be appropriate.

When it comes to providing a 'default' of any type I tend to think it's best to store those specs in-cluster. This allows specs to be updated and new specs added with out worrying about the ENV of every user that could be accessing the cluster. For example, if a support team are all working on a single environment having default specs and redactors stored in the cluster ensure they all have the same default using the "collect from cluster" which might include multiple specs from multiple URLS.

Having said that I also want to point out our documentation seems to be very lacking in this area as well. I can't find good references to what I'm saying above on troubleshoot.sh and that's a problem.

[camilamacedo86]

Hi @chris-sanders,

(imo) To address what motivates me open this issue would be only (low effort):

improve the CLI by adding a further info for usage and add examples (&cobra.Command{ Example: <string const to output> }, i.e.).

AND

• a) Add a better error to the arg (the usage/examples also could describe what/why it is required) (PS.: Why only this input is an arg and not a flag like the others? ) OR
• b) Move to flag and mark it as mandatory (i.e. MarkFlagRequired("spec")) so that we would have a desc for that and it would result in more like what I was expecting to see when I executed kubectl preflight,i.e:

Error: required flag(s) "spec" not set
Usage:
   [flags]

Flags:
  -b, --spec string   inform the the URL or path for your preflight check spec. (i.e. https://preflight.replicated.com)
  -h, --help         help for this command

Question: how the end-user can find out what is the right spec to be used for each context without see docs? (idea): It seems that a good option in long term could be the CLI have a config context such as kubectl does (with kubeconfig). So, we could have different default values per context. That could allow solutions that leverage on this project set it out (I am not sure if that is a valid effort now either)

---

TL'DR: Following the answers and further details.

Can you explain why you would prefer a mandatory user input be a flag instead of an argument?

I suggested mainly because:

• by using flags, we have help and useful info about what should be set there, etc.
• we can also make flags mandatory and/or validate their value and return the error
• this project/cli uses flag for all less this one, see the current behaviour with preflight:

1. You run:

$ kubectl preflight
Error: requires at least 1 arg(s), only received 0

1. Then, you do not know what is missing. You hope that the cli has a help besides it not be returned above as usual.
2. You run --help you do not found any info at all about this.

./bin/preflight --help
A preflight check is a set of validations that can and should be run to ensure
that a cluster meets the requirements to run an application.

Usage:
  preflight [url] [flags]
  preflight [command]

Available Commands:
  completion  Generate the autocompletion script for the specified shell
  help        Help about any command
  version     Print the current version and exit

Flags:
      --as string                      Username to impersonate for the operation. User could be a regular user or a service account in a namespace.
      --as-group stringArray           Group to impersonate for the operation, this flag can be repeated to specify multiple groups.
      --as-uid string                  UID to impersonate for the operation.

Similarly, why would you choose this as an ENV specifically. Would you make all CLI flags available as ENV variables as well or only this one?

I did not check/analyse all possible flags. However, it seems possible to use viper (https://github.com/spf13/viper) with cobra to win ENV VARs from the flags and have this in a more low effort(easier to keep maintained) way.

Use env vars seems an easy option to allow set default values per environment (context) instead of ask for end-users looking for what is the required value for each option.

I'm not aware of any standard pattern of CLI utilities using ENV variables for inputs

Are you asking for examples? If so:

• docker CLI provides ENV VARs to allow set context info for the builds: https://docs.docker.com/engine/reference/commandline/cli/#environment-variables
• K8s Kops also seems to using viper to provide ENV VARs for the flags: https://github.com/kubernetes/kops/search?q=viper

[banjoh]

Would adding the following two changes improve the situation?

• Improve the usage description to show what the url ought to be
• Show the --help output whenever there are errors parsing cli flags

Example

✔ ~/repos/replicated/troubleshoot [em/sbctl-pod-logs-744 {origin/em/sbctl-pod-logs-744}|✚ 1…1⚑ 1]
[evans] $ ./bin/preflight
A preflight check is a set of validations that can and should be run to ensure
that a cluster meets the requirements to run an application.

The [url] argument is either an oci://.., https://.. or a local path to a yaml file.

Usage:
  preflight [url] [flags]
  preflight [command]

Available Commands:
  completion  Generate the autocompletion script for the specified shell
  help        Help about any command
  version     Print the current version and exit

Flags:
      --as string                      Username to impersonate for the operation. User could be a regular user or a service account in a namespace.

<truncated>

Use "preflight [command] --help" for more information about a command.
✘-1 ~/repos/replicated/troubleshoot [em/sbctl-pod-logs-744 {origin/em/sbctl-pod-logs-744}|✚ 1…1⚑ 1]

[chris-sanders]

I suggested mainly because:

by using flags, we have help and useful info about what should be set there, etc.

This doesn't require an argument as shown above. And requiring a flag simply adds additional typing without solving the error handling.

we can also make flags mandatory and/or validate their value and return the error

We can do both of those things without a flag as well.

this project/cli uses flag for all less this one, see the current behaviour with preflight:

That's because the other items aren't mandatory. This is mandatory and it's an argument because without a flag to alter the default operation you must provide it. The spec you want to run is the primary information you need to provide to troubleshoot and seems like an appropriate argument 0 to me.

I like the suggestion from banjoh. I think it clearly demonstrates that an argument can address all of these concerns. I'm mostly focusing on UX here, I'm only seeing downsides to moving a mandatory argument to a flag for user experience. I don't see any technical benefits as all issues you're raising apply equally to flags and arguments.

---

Are you asking for examples? If so:

docker CLI provides ENV VARs to allow set context info for the builds: https://docs.docker.com/engine/reference/commandline/cli/#environment-variables K8s Kops also seems to using viper to provide ENV VARs for the flags: https://github.com/kubernetes/kops/search?q=viper

Not really, CLI programs are a dime a dozen and we can find examples of just about anything you want. What I really meant was is there a design pattern which suggests CLI programs should accept ENV as inputs that describes when and how you decide which ones to include. I'm not sure I'm included to say we should make all flags map to ENV as well. To me that feels like we're starting to introduce 'magic' where something happens different from what the user intended because an ENV modified it.

For example, if we post a command to a user to collect information and that command is modified by the environment they are in it can cause confusion. Aliases are a perfectly serviceable way for an individual to make shortcuts to explicit commands they want to run without introducing the element of surprise where it gets triggered unintentionally.

But that's just my opinion on the subject, and if there's a design principal with clear instructions on why it's a good pattern I'd be happy to review that maybe that changes my opinion.

[banjoh]

Its worth noting that the analyze command differs from the rest

./bin/analyze --analyzers ./examples/support-bundle/sample-analyzers.yaml

[chris-sanders]

Hu that's interesting, I wonder if we should revisit that. Should support bundle expect a spec, pre-flight expect a spec, and analyze expect a spec?

[banjoh]

Hu that's interesting, I wonder if we should revisit that. Should support bundle expect a spec, pre-flight expect a spec, and analyze expect a spec?

I think that's a good objective, We'd then have ./bin/<cmd> <spec> <options>

[camilamacedo86]

Hi @chris-sanders,

CLI programs are a dime a dozen and we can find examples of just about anything you want. What I really meant was is there a design pattern which suggests CLI programs should accept ENV as inputs that describe when and how you decide which ones to include

Usually, we can use ENV VARs for values that are based in the environment so which allows us to have configuration by envs and improve the UX.

Then, about a doc and guideline for good practices, I think the most comprehensive one that I found so far is: https://clig.dev/. I think we could track it internally indeed as our dev practices and try to follow up across all projects. WDYT?

Therefore, about the topics raised here:

• About flags and args: https://clig.dev/#arguments-and-flags ( it is preferable to use flags instead of args)
• About ensuring the same behaviour across: https://clig.dev/#consistency-across-programs (we need also to ensure consistency as pointed out by @banjoh )
• About help and the need to improve it: https://clig.dev/#help
• About env variables: https://clig.dev/#environment-variables ( that describe less the same discussed above, it is made easier and improves the UX and should be used for values that can be changed by context/env )

Then, by looking at https://clig.dev/#configuration I understand that the best option for us/the recommendation is to have a config file (such as kubectl has the context)

[chris-sanders]

Thanks, that's a really good reference!

About ensuring the same behaviour across: https://clig.dev/#consistency-across-programs (we need also to ensure consistency as pointed out by @banjoh ) About help and the need to improve it: https://clig.dev/#help

I think this is the heart of the actual issue and we're all in agreement here. Lack of consistency between the CLI programs and any missing help files are technical debt not intentional decisions.

@banjoh I think we're in alignment on the pattern ./bin/<cmd> <spec> <options> is that true today or would you mind opening an issue to track work that you're aware of to make that true?

---

This becomes a bit off topic of the original issue we might want to carry further discussion of this in a new issue since the help and consistency is, I believe, in agreement. I'll continue here because we might not have a lot more to close this out.

About flags and args: https://clig.dev/#arguments-and-flags ( it is preferable to use flags instead of args)

I think that's a good policy too. I think we're only proposing a single argument in this case and it's an always required mandatory argument. We also accept multiples of that same input which is given as an explicit example of when it's fine to have arguments. I think our use of this argument matches with the guidelines here.

About env variables: https://clig.dev/#environment-variables ( that describe less the same discussed above, it is made easier and improves the UX and should be used for values that can be changed by context/env )

I like the way they describe this. Environment variables are for behavior that varies with the context in which a command is run.

The examples provided for things like Shell are a good example, and I landed a change to sbctl specifically for this. I don't think most of our configuration values qualify as varying per environment. The troubleshoot tools, like kubectl, are run on a machine and can access many different clusters. What you collect from those clusters varies but the local environment won't and actually can't match multiple different clusters.

Because of this I don't think most of our flags and certainly not our single argument are good candidates for Environment variables.

[banjoh]

I'm fleshing out https://github.com/replicatedhq/troubleshoot/issues/877 issue with particulars of what should/could change

[banjoh]

I believe we can close this issue

[banjoh]

It just occurred to me that we did not address the original problem which was ambiguity of the error message seen in a CLI is run with no arguments.

Are we happy with printing the usage text if there are no arguments passed in as I had described https://github.com/replicatedhq/troubleshoot/issues/824#issuecomment-1317338529?

[chris-sanders]

Yes that seems like a reasonable solution.