Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cli): exit code 1 on unknown commands, sub-commands and flags #5235

Merged
merged 2 commits into from Oct 17, 2023

Conversation

vvagaytsev
Copy link
Collaborator

@vvagaytsev vvagaytsev commented Oct 13, 2023

What this PR does / why we need it:
This PR brings strict consistency into Garden command exit codes and fixes the possible CI problems like the ones we got in #4868.

Which issue(s) this PR fixes:

Fixes #4911

Special notes for your reviewer:
This PR has been done in TDD mode :)
The test specs were written first.

To be back-ported to 0.12.

TODOs (to be addressed in a separate PR):

  • ensure the error message is always printed on unrecognized flag
  • ensure the error message is always printed on unrecognized command or sub-command

@vvagaytsev vvagaytsev force-pushed the fix/4911 branch 2 times, most recently from 799a0f1 to b6d9086 Compare October 13, 2023 15:13
@vvagaytsev vvagaytsev changed the title fix(cli): exit code 1 on bad commands, sub-commands and flags fix(cli): exit code 1 on unknown commands, sub-commands and flags Oct 13, 2023
@vvagaytsev vvagaytsev force-pushed the fix/4911 branch 5 times, most recently from 90d6d1b to 57db35a Compare October 16, 2023 12:27
@vvagaytsev vvagaytsev requested review from stefreak and a team October 16, 2023 14:47
@vvagaytsev vvagaytsev marked this pull request as ready for review October 16, 2023 14:47
stefreak
stefreak previously approved these changes Oct 16, 2023
Copy link
Member

@stefreak stefreak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! I have one minor comment on a comment.

I like the tests a lot ❤️

core/src/cli/cli.ts Outdated Show resolved Hide resolved
@stefreak
Copy link
Member

One comment on the plan to backport this to 0.12: Do we have to do that? I think it might not be important enough to justify the effort (But I might be wrong, you might have good reasons).

@vvagaytsev
Copy link
Collaborator Author

One comment on the plan to backport this to 0.12: Do we have to do that? I think it might not be important enough to justify the effort (But I might be wrong, you might have good reasons).

It's not super critical, but it might be better to align the exit code behavior between 0.12 and 0.13. The back-port should be a big effort. We're going to do the next 0.12 release soon, so we can include this fix too.

@vvagaytsev vvagaytsev added this pull request to the merge queue Oct 17, 2023
Merged via the queue into main with commit 66007f2 Oct 17, 2023
42 checks passed
@vvagaytsev vvagaytsev deleted the fix/4911 branch October 17, 2023 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Garden exists with 0 exit code on unsupported options and sub-commands
2 participants