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: add exit codes to different flag validation errors #861

Merged
merged 11 commits into from
Dec 4, 2023
Merged

Conversation

WillieRuemmele
Copy link
Contributor

@W-14408666@

adds different exit codes (3-7) for different flag/args validation errors

Copy link
Contributor

@mdonnalley mdonnalley left a comment

Choose a reason for hiding this comment

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

@WillieRuemmele I think this needs to be configurable at the CLI level so we don't force all CLIs to use our standards. I'd like to keep oclif as un-opinionated as possible

I'm thinking that the CLI could have something like this in the package.json

{
  "oclif": {
    "exitCodes": {
        "invalidArgs": 3,
        "requiredFlag": 4,
        // etc...
      }
  }
}

You might have an issue with accessing the root plugin's package.json from parser/validate.ts. If so, you could consider putting all the exit codes on the Cache class and then access them there - that way you don't have to pass around the package.json through multiple levels

mshanemc
mshanemc previously approved these changes Nov 22, 2023
Copy link
Member

@mshanemc mshanemc left a comment

Choose a reason for hiding this comment

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

approved once comment stuff is cleaned up.

@@ -341,8 +341,10 @@ export class Parser<

return await flag.parse(input, ctx, flag)
} catch (error: any) {
error.message = `Parsing --${flag.name} \n\t${error.message}\nSee more help with --help`
throw error
// console.log(error)
Copy link
Member

Choose a reason for hiding this comment

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

don't leave the comments in

arg1: Args.string({
required: true,
}),
// qux: Args.string({
Copy link
Member

Choose a reason for hiding this comment

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

did you need this?

@mshanemc
Copy link
Member

QA notes:

build oclif/core and yarn link into the local CLI (from installer) /Users/shane.mclaughlin/.local/share/sf/client/current

run a command, then echo $? to see exitCode.
sf version --foo => 2
saying "no" to did-you-mean after entering a non-existent command => 127
sf org create sandbox (missing flag) => 1
sf org list --no-prompt (missing dependsOn) => 1

I think these are thrown from "inside" a command instead of during parse/validate
sf update --version 0.0.0 => 1
sf org display -o bad => 1


and then we go customize the exitCodes prop

"nonExistentFlag": 404, ❓ sf version --foo => 148 [I don't know how it gets to 148 from 404. I think it's modulo 255?
"nonExistentFlag": 8, ✅ sf version --foo => 8
"nonExistentFlag": 33, ✅ sf version --foo => 33

"failedFlagValidation": 5, sf org list --no-prompt (missing dependsOn) => 1 ❌ should have been 5.
[I tried a bunch of things and could never get failedFlagValidation to fire from dependsOn]

@mdonnalley
Copy link
Contributor

"nonExistentFlag": 404, ❓ sf version --foo => 148 [I don't know how it gets to 148 from 404. I think it's modulo 255?

Turns out that exit codes beyond 255 aren't supported: https://www.shellscript.sh/exitcodes.html

@mdonnalley
Copy link
Contributor

mdonnalley commented Nov 29, 2023

"failedFlagValidation": 5, sf org list --no-prompt (missing dependsOn) => 1 ❌ should have been 5.
[I tried a bunch of things and could never get failedFlagValidation to fire from dependsOn]

This is an issue with any command extending SfCommand. I'll have to fix that in a separate PR

@mshanemc mshanemc merged commit 1c841bf into main Dec 4, 2023
59 checks passed
@mshanemc mshanemc deleted the wr/exitCodes branch December 4, 2023 19:57
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.

None yet

3 participants