feat(cmd): Adding fail-non-empty flag6132 #6153
Conversation
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
cmd/exec.go
Outdated
| cmd.Flags().BoolVarP(¶ms.Fail, "fail", "", false, "exits with non-zero exit code on undefined/empty result and errors") | ||
| cmd.Flags().BoolVarP(¶ms.FailDefined, "fail-defined", "", false, "") | ||
| cmd.Flags().BoolVarP(¶ms.Fail, "fail", "", false, "") | ||
| cmd.Flags().BoolVarP(¶ms.Fail, "fail-non-empty", "", false, "") |
There was a problem hiding this comment.
This should not use ¶ms.Fail but use the FailNonEmpty param added for the non-empty case.
Also, removing all documentation from these options seems wrong. I'd go with:
fail-defined: "exits with non-zero exit code on defined result and errors"
fail: "exits with non-zero exit code on undefined result and errors"
fail-empty: "exits with non-zero exit code on empty result and errors"
cmd/exec_test.go
Outdated
| description: "--fail-non-empty with true boolean result", | ||
| files: map[string]string{ | ||
| "files/test.json": `{"foo": 7}`, | ||
| "bundle/x.rego": `package fail.defined.flag |
There was a problem hiding this comment.
These package names are misleading. Maybe use something like fail.non.empty.flag instead?
Thank you for the review, I will check them out. |
|
/retest |
|
Could you help me to understand the failure at https://github.com/open-policy-agent/opa/actions/runs/5897177383/job/15996426056?pr=6153#step:6:258? Thanks. |
cmd/exec.go
Outdated
| cmd.Flags().BoolVarP(¶ms.Fail, "fail", "", false, "exits with non-zero exit code on undefined/empty result and errors") | ||
| cmd.Flags().BoolVarP(¶ms.FailDefined, "fail-defined", "", false, "exits with non-zero exit code on defined result and errors") | ||
| cmd.Flags().BoolVarP(¶ms.Fail, "fail", "", false, "exits with non-zero exit code on undefined result and errors") | ||
| cmd.Flags().BoolVarP(¶ms.FailNonEmpty, "fail-non-empty", "", false, "exits with non-zero exit code on non empty result and errors") |
There was a problem hiding this comment.
Nit: but we say "non-zero", so we should say "non-empty" too.
There was a problem hiding this comment.
Thanks for the comment, I have modified it at this commit f91f52d
Signed-off-by: Ronnie Personal <76408835+Ronnie-personal@users.noreply.github.com>
Signed-off-by: Ronnie Personal <76408835+Ronnie-personal@users.noreply.github.com>
…st case Signed-off-by: Ronnie Personal <76408835+Ronnie-personal@users.noreply.github.com>
Signed-off-by: Ronnie Personal <76408835+Ronnie-personal@users.noreply.github.com>
anderseknert
force-pushed
the
failNonEmpty6132
branch
from
f91f52d
to
c1e490f
Compare
|
I've now tried this out locally and it seems like it's working as expected. Thanks for you work on this @Ronnie-personal 👍 |
Why the changes in this PR are needed?
This PR addresses issue #6132
What are the changes in this PR?
'--fail-non-empty' flag is added to 'opa exec' CLI.
Notes to assist PR review:
Further comments: