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

Return non-zero exit status on no valid objects #209

Merged
merged 3 commits into from
Mar 18, 2022
Merged

Return non-zero exit status on no valid objects #209

merged 3 commits into from
Mar 18, 2022

Conversation

ytsarev
Copy link
Contributor

@ytsarev ytsarev commented Sep 13, 2021

Instead of printing the warning and return 0 exit status
return Error and exit with 1

Reasoning: our kube-linter pipelines where silently failing
for a while without any notice because of 0 exit status

See k8gb-io/k8gb#615 (comment)

Signed-off-by: Yury Tsarev [email protected]

Copy link
Contributor

@viswajithiii viswajithiii left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Comment on lines 98 to 99
fmt.Fprintln(os.Stderr, "Warning: no valid objects found.")
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

This breaks backward compatibility, and is more in line with how I'd expect a linter to work -- just emit a warning if there are no valid objects, but don't fail altogether. I would be open to adding a flag, though, that controls this, so that it someone passes it in (say, "--fail-if-no-objects-found"), then we return an error. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I clearly missed that one, will try to make a flag, thanks for the review!

@jkremser
Copy link

@viswajithiii I've added the --fail-if-no-objects-found flag and some testing, could you please take a look? The CI is failing on something unrelated.

I've run the new e2e tests locally by

KUBE_LINTER_BIN=~/workspace/kube-linter/bin/darwin/kube-linter go test e2etests/sanity_test.go

..and they passed.

We use the kube-linter via gh action and It would be nice to have this in upstream, otherwise we have to do some ugly workarounds like this.

ytsarev and others added 3 commits February 10, 2022 14:35
Instead of printing the warning and return 0 exit status
return Error and exit with 1

Reasoning: our kube-linter pipelines where silently failing
for a while without any notice because of 0 exit status

See k8gb-io/k8gb#615 (comment)

Signed-off-by: Yury Tsarev <[email protected]>
@jkremser
Copy link

rebased on main with the hope that the CI will pass

@ytsarev
Copy link
Contributor Author

ytsarev commented Mar 17, 2022

@viswajithiii hey, how does it look to you now? I think @jkremser addressed the issue the best possible way

Copy link
Contributor

@viswajithiii viswajithiii left a comment

Choose a reason for hiding this comment

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

Thank you! Sorry for the late review, I missed the previous notification.

@viswajithiii viswajithiii merged commit 6d4491d into stackrox:main Mar 18, 2022
@jkremser jkremser deleted the error-on-no-valid-objects branch March 18, 2022 14:44
@jkremser
Copy link

@viswajithiii thanks for merging 👍

may I ask if there will be a release with the change? Or if there is a way to specify the version of the kube-linter tool from kube-linter-action to something like gitsha or branch/tag. It looks like the version can be set to latest, but this means the "latest released", right?

@viswajithiii
Copy link
Contributor

Hi @jkremser, yup, it's been a while since our last release, so I was thinking of cutting one. @janisz, would you be interested in doing the release cut itself?

@janisz
Copy link
Collaborator

janisz commented Mar 18, 2022

Sure I can handle this after the weekend

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.

4 participants