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 CLI flag to override exit code if API is unreachable #99

Merged
merged 1 commit into from
Feb 23, 2024

Conversation

martialblog
Copy link
Member

@martialblog martialblog commented Feb 19, 2024

Fixes #98

TODO

  • Agree on a name for the new flag
  • Update README

@martialblog martialblog self-assigned this Feb 19, 2024
@martialblog
Copy link
Member Author

@RincewindsHat Not sure about the flag's name yet. Ideas?

@martialblog
Copy link
Member Author

I'd like to have a generic CLI flag name so that we can re-use it in different plugins

@RincewindsHat
Copy link
Member

critical-if-unreachable for a really verbose version
unreachable-state $STATE for setting the state on the caller side

@martialblog martialblog marked this pull request as ready for review February 20, 2024 14:08
@martialblog
Copy link
Member Author

Would be nice to have an pflag Enum Type, but that's currently not available. Our go-check module fortunately is clever enough to handle things other than 0,1,2,3

@RincewindsHat
Copy link
Member

Would be really nice to be able to use "2", "Critical", "critical" or "CRITICAL" equally.

@martialblog
Copy link
Member Author

But then we would have to have a StringVarP, and then validate if it's an "accepted" (warn, crit, unknown) string, then map the string to an int... I don't see any added value to that?

@RincewindsHat
Copy link
Member

Usability? How many people do actually know how the statuses map to integers?

@martialblog
Copy link
Member Author

I'd assume that people in the monitoring plugins ecosystem are at least familiar with exit codes and their meaning. I would prefer to keep the code simple and people can read up on stuff IF they need this feature at all

Also - and I realize that this point is far fetched - but what if another monitoring plugin system does not use the same "warning" "critical" "unkown" mapping that Icinga does.

@RincewindsHat
Copy link
Member

ALL the nagios decendents use the same mapping to my knowledge. If you change that you can just stop using these monitoring plugins at all.
They would be pretty much useless.

But this option can be expanded in the future without breaking the API to the user, so I would say we go for it now with numerical codes only.

I would request a enhancement of the help text for the option though to make it clearer what the 3 means and also maybe what the other options are.

@martialblog
Copy link
Member Author

Updated the help text and README

@martialblog martialblog merged commit d9a57cc into main Feb 23, 2024
2 checks passed
@martialblog martialblog deleted the feature/health-crit branch February 23, 2024 07:19
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.

[Feature]: connection refused (*url.Error) as CRITICAL or UNKNOWN
2 participants