Fix: Handling of --help arg for sub commands

[marcleblanc2]

Usage / helper text was only printed for either src (top-level) or src <command> (command-level), ignoring any of the usage functions defined by subcommands.

For example, src snapshot upload --help would only output the usageFunc for src snapshot

With this change:

Any of the following will output the usageFunc for src snapshot upload

src snapshot upload --help
src snapshot --help upload
src --help snapshot upload

src snapshot upload -h
src snapshot -h upload
src -h snapshot upload

src snapshot upload help
src snapshot help upload
src help snapshot upload

Any of the following will output the usageFunc for src snapshot

src snapshot --help
src --help snapshot

src snapshot -h
src -h snapshot

src snapshot help
src help snapshot

Any of the following will output the usageFunc for src

src --help

src -h

src help

Test plan

• Manual testing via Bash script
• Added unit tests
• Amp's pretty happy with itself
• The issues remediated in Fix #1105: Execute commands with zero arguments correctly #1117 are still remediated; commands that do not require args still run
• The functionality change from Consistently redirect usage examples to stdout #1105 is still behaving as needed; helper text is printed to stdout when expected / when the user runs a command with a --help / -h / help arg, but prints to stderr when printed in response to invalid user input

[marcleblanc2]

Hey @bobheadxi or @keegancsmith, I'd like this to make it into the 6.10 release on Wednesday, if either of you has a moment to review, or lmk who else I should ask 🙏

[keegancsmith]

This is extremely complicated compared to what we had before. Looking at the code in src-cli it uses go's built in 'flag' apis. That only supports -h. If you do something like

src snapshot upload -h

it works correctly. The problem comes in is that we added in extra support for --help and reading that code you can see that is naively only supports the first level of subcommands, not deeper ones like src snapshot upload --help. Fixing that is the proper fix here.

I know you want to get this in, so I sent out this alternative fix. Mind testing it and seeing if it is acceptable? It doesn't exactly match all the use cases you mentioned, but should solve the main complaint you have. #1195

[keegancsmith]

the pattern in this file of using subprocesses is very complicated for what you want to check. I'm guessing you did it because of the os.Exit in the code under test. Rather just make os.Exit something you can inject?

[marcleblanc2]

I'm not aware of the difference, but Amp had picked this route so that it could validate the exit code.

src snapshot upload -h would return 0, whereas src snapshot upload would return 2 (input error).

Validating the return code is not the most material change, so if it's more complex than it's worth, it's fine to remove.

[keegancsmith]

note: the introduction of all these comments does not make the code more readable and makes the PR much harder to review.

[marcleblanc2]

Noted. I felt the code was rather cryptic to read.

[keegancsmith]

Alternative was merged in.