chocolatey / choco

Chocolatey - the package manager for Windows
https://chocolatey.org
Other
10.05k stars 890 forks source link

Verify command line flags (by default) #3371

Open sdarwin opened 6 months ago

sdarwin commented 6 months ago

Checklist

Is Your Feature Request Related To A Problem? Please describe.

It's easy to mistype any command. When that happens, it's helpful to get an informational error.

Here is an example:

choco install visualstudio2022buildtools --fake-command-line-flag

Chocolatey doesn't indicate the option is invalid?

Or

choco install visualstudio2022buildtools --package-parametersss

Almost correct. The same problem. No warning message. No error message. Just a "success".

Describe The Solution. Why is it needed?

Sanitize command line flags. Return a clear error message "invalid option : X" . Exit with an error code.

It's better to return an error, than to allow the user to mistakenly believe they are getting certain options, but the options were misspelled.

(the proposal is that this should occur 'by default'.)

Additional Context

No response

Related Issues

No response

gep13 commented 6 months ago

@sdarwin sounds like you want to make use of the ignoreInvalidOptionsSwitches feature flag.

https://docs.chocolatey.org/en-us/configuration#ignoreinvalidoptionsswitches

This is enabled by default, since there are ways of using Chocolatey CLI where you specifically want to pass in options that Chocolatey CLI doesn't know about.

However, for your use case, if you run:

choco feature disable --name="ignoreInvalidOptionsSwitches"

You should get the result you want.

Let me know if this answers your question, and we can close out this issue.

sdarwin commented 6 months ago

Hi @gep13 ,

This was implemented in issue 586. Comments from there:

like the way PowerShell ignores switches it doesn't understand

Well, if I run a powershell script it will give an error:

script.ps1 -helpz

A parameter cannot be found that matches parameter name 'helpz'

This allows passing things that would be created in future versions and not having older versions break when attempting to use those switches.

Usually if a new switch gets added in a future version, it becomes available in future versions. Older versions don't have access to that particular switch. But that is normal.

What is the most frequent and common case of needing to pass unknown options to choco? If 97% of users are not applying unknown options flags, those users would be better off if the default setting was catching typos. Rather than allowing mistakes to go undetected.

It would sort of make sense if this was the method to pass options directly to the underlying installer. However, to do that, you use --install-argumentsor --package-parameters.

gep13 commented 6 months ago

@sdarwin said... What is the most frequent and common case of needing to pass unknown options to choco?

One example would be the choco new command, where you can pass additional arguments that are passed in as templated variables. This is done in conjunction with the templating functionality that exists in Chocolatey CLI.

@sdarwin said... If 97% of users are not applying unknown options flags, those users would be better off if the default setting was catching typos.

Due to how Chocolatey CLI works, i.e. no phone home, or telemetry, we simply don't know what the usage is.

If I have understood you correctly, what you are requesting is no longer that Chocolatey CLI detects that invalid options are passed in (since this functionality is already provided via the ignoreInvalidOptionsSwitches feature), but rather you are requesting that the default value of this feature be changed. If that is indeed the case, can I request that you go back and update the issue title and description to reflect that request.

sdarwin commented 6 months ago

If "new" must be able to take arbitrary switches, then treat "new" differently. Allow that command to have unknown options.

We can imagine, that most users are running "choco install", and most users don't know about the existence of "ignoreInvalidOptionsSwitches".

Default settings should be aimed at the largest set of users, the typical user.

There's a reason in Linux if you type ls --test that it says

ls: unrecognized option '--test'
Try 'ls --help' for more information.

Error code 2.

The problem with allowing unknown options is more than typos, it's also "misunderstandings". You construct a long, somewhat complicated installation command, believe it should be working, and there are no errors or warnings. Yet behind-the-scenes, it failed to do what you expected.

While I believe the command should output an error message and exit with an error code, like ls, there is compromise position. Display a warning message. Continue with the current behavior of attempting to run the command.


(intentionally left blank. add spaces, colors, or some way to call attention to the message)

WARNING. choco does not recognize the option '--test'. That option will not be processed. Attempting to proceed anyway.

Perhaps implement the warning-only mode first and leave that in place for a couple years. Then eventually enable errors. A phased approach.

If there is a case to be made for certain subcommands such as new those could be exceptions.

github-actions[bot] commented 5 months ago

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? This issue will be closed in 14 days if it continues to be inactive.

sdarwin commented 5 months ago

This issue will be closed in 14 days if it continues to be inactive.

The "default" behavior of a command line tool should be to verify the provided --option-flags. If 10% or fewer subcommands really do need to accept any input, it could be triggered by that particular subcommand itself.

So let's say -z needs to be able to accept a bunch of unknown unvalidated command-line input. The presence of -z unlocks that behavior. Otherwise validate inputs.

pauby commented 5 months ago

@sdarwin @gep13 asked:

If I have understood you correctly, what you are requesting is no longer that Chocolatey CLI detects that invalid options are passed in (since this functionality is already provided via the ignoreInvalidOptionsSwitches feature), but rather you are requesting that the default value of this feature be changed. If that is indeed the case, can I request that you go back and update the issue title and description to reflect that request.

(emphasis mine). That hasn't been done, and that is why the issue was due to be closed.

To set expectations, the default behaviour of this option is already established behaviour, and changing it would cause unexpected issues for some users. We would appreciate any feedback on how we highlight that this option being available, to more people.

If I've misunderstood what you are asking, please let me know.

sdarwin commented 5 months ago

That hasn't been done, and that is why the issue was due to be closed.

I had already update the title, from "Verify command line flags" to -> "Verify command line flags (by default)".

Now I have added text into the first post: "(the proposal is that this should occur 'by default'.)

Implementing this feature request is more then toggling ignoreInvalidOptionsSwitches because gep13 mentioned choco new as an example of a command that might require unknown switches. Are there other examples of commands where unknown switches must be used? If so, those few cases would need to "ignore invalid options switches".

If there are currently any seldom-used, mysterious, but still necessary, switches that aren't documented, they should be added into the list of "valid command-line switches" in some way or another.

But generally I suspect that 95% of users are running "choco install", and 95% of users don't know about ignoreInvalidOptionsSwitches, and those same users will never know about ignoreInvalidOptionsSwitches.

Still the "average use case" would benefit from getting at least a warning when there's a problem with the command-line input.

I had suggested a compromise. Instead of having choco crash, display a clear warning message. Leave the warning message in place for a couple years. Before eventually continuing along the path of generating an error.

pauby commented 5 months ago

As I mentioned above, the behaviour of Chocolatey CLI is already established, whether it's believed to right or wrong. So it's not something we'd be changing.

Outside of making that change, I'm unclear what else needs to do be done here.

sdarwin commented 5 months ago

In that case...

How about a slightly different alternative then.

Instead of modifying ignoreInvalidOptionsSwitches, add a new setting, something like "enable warnings".

"enableWarningsInvalidOptionsSwitches". True by default.

It will "show warnings when invalid options switches are found". at least in my opinion, since the warnings are sort of serious, they should be offset by surrounding spaces. Try to draw attention to them.