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

Fail on warn should exit with different rc #300

Closed
nirroz93 opened this issue Apr 30, 2020 · 6 comments · Fixed by #378
Closed

Fail on warn should exit with different rc #300

nirroz93 opened this issue Apr 30, 2020 · 6 comments · Fixed by #378
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@nirroz93
Copy link

Hi!

Currently, running conftest test --fail-on-warn return 1 when there is a warning or an error.

Different return code when there are no errors (just warn) will be nice and, for me, it seems like the "normal" behaviour (meaning: when I was surprised it didn't :-) )

@garethr
Copy link
Contributor

garethr commented May 1, 2020

I like the idea. What exit codes would you propose?

@garethr garethr added enhancement New feature or request good first issue Good for newcomers labels May 1, 2020
@nirroz93
Copy link
Author

nirroz93 commented May 4, 2020

IMHO the most intuitive rc 0 OK 1 WARN (with no errors) 2 ERROR (and error+warn), or leave some place for other future levels, but this may cause backward compatibility if someone is using the current return code (and I don't see a reason for using ==1 and not !=0, but if you think there is so 1 error 2 warn).

@jpreese
Copy link
Member

jpreese commented May 6, 2020

@nirroz93 I'd be curious about your use case. I'm not necessarily opposed to providing more information to the user about the results, but admittedly I do like the current behavior.

Without --fail-on-warn in CI you'll most likely want WARN to continue to process, and on your local computer, you'll see the warnings in the output window.

With --fail-on-warn in CI your warnings will fail, which should be expected given the argument name.

@nirroz93
Copy link
Author

nirroz93 commented May 6, 2020

@jpreese
I want to do other action items for warns and error. For example, on warn I want to put the Jenkins job as "unstable"+mail, but for other deny (from other checks) I want to fail the job.

In the current state I need to catch the output and parse it (or run conftest twice).

@jpreese
Copy link
Member

jpreese commented May 6, 2020

When possible, I like to mimic the behavior of OPA. I feel the experience shouldn't diverge too much, because contest just builds onto all the niceties that OPA provides.

That said, in their eval command they have 0 = success, 1 = undefined, 2 = error. For our purposes, we could consider "undefined" as no errors exist, but a warning was found. (open-policy-agent/opa#981)

I would agree with you @nirroz93 that the best course of action would most likely be:

(without --fail-on-warn)
0 -> warnings. no errors.
1 -> error occurred

(with --fail-on-warn)
0 -> no warnings or errors
1 -> no error occurred. has at least 1 warning.
2 -> an error occurred. (could also have warnings)

I would suspect most CI checks for != 0, so this has low impact to breaking current implementations, gives you the data you need, and is at least somewhat in line with OPA.

@jpreese
Copy link
Member

jpreese commented Sep 6, 2020

Resolved via #378

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants