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

Add exit codes to the tool #20

Merged
merged 1 commit into from
Mar 18, 2024
Merged

Add exit codes to the tool #20

merged 1 commit into from
Mar 18, 2024

Conversation

arbulu89
Copy link
Contributor

@arbulu89 arbulu89 commented Mar 14, 2024

Add exit codes to the tool. It doesn't change much the logic, except it handles the unauthorized scenario. When one unauthorized error is received, the execution finishes, as the rest will have the same result.
All remaining errors are not treated, and it still returns 0 exit code.

  • Everything OK -> 0
  • Unauthorized/Non existing scenario -> 1
  • Subcommand not used -> 64

PD: I just was bored, so I thought on doing this. We can entirely ditch if it is so bad hehe

@arbulu89 arbulu89 added the enhancement New feature or request label Mar 14, 2024
@arbulu89 arbulu89 marked this pull request as ready for review March 14, 2024 14:43
Copy link
Collaborator

@dottorblaster dottorblaster left a comment

Choose a reason for hiding this comment

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

LGTM

run::run(endpoint_url, api_key, scenario_label.to_string(), scenarios).await;
match run::run(endpoint_url, api_key, scenario_label.to_string(), scenarios).await {
Ok(()) => std::process::exit(exitcode::OK),
Err(()) => std::process::exit(1)
Copy link
Collaborator

@dottorblaster dottorblaster Mar 14, 2024

Choose a reason for hiding this comment

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

I'm pretty surprised exitcode doesn't have a 1 exit code inside, I know it just wraps value from sysexits.h but it feels a little clumsy 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah man, i don't know why it doesn't include it. I was just to about to remove the complete package usage only for that hehe

@arbulu89 arbulu89 merged commit fe8138a into main Mar 18, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

2 participants