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

fix: Return error exit code when collectors fail #527

Merged
merged 1 commit into from
Sep 8, 2023

Conversation

dark0dave
Copy link
Collaborator

Error, errors out properly closes #450 and increased log level for silent resource failures

@dark0dave dark0dave added this to the 0.8.0 milestone Sep 5, 2023
@dark0dave dark0dave self-assigned this Sep 5, 2023
@dark0dave dark0dave force-pushed the feature/authError branch 3 times, most recently from d647824 to a5f43d6 Compare September 5, 2023 13:14
Copy link
Contributor

@stepanstipl stepanstipl 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 this 👍

The main thing is the desired error behaviour -
I believe that when there's a failure with one of the enabled collectors, we should always exit with an error, irrespective of the config.ExitError option. That option/flag is meant to configure whether there should be an error when issues are found - i.e. kubent operates successfully, but has found some deprecated resources. If we fail to operate successfully, we should always exit with error.

So in this case, I would error always, irrespective of that flag.

Some minor comments in the code on implementation and the logging bit.

Plus can we cover this with a test? Perhaps it could be another one in

func TestMainExitCodes(t *testing.T) {

cmd/kubent/main.go Outdated Show resolved Hide resolved
cmd/kubent/main.go Outdated Show resolved Hide resolved
pkg/collector/cluster.go Outdated Show resolved Hide resolved
@dark0dave dark0dave force-pushed the feature/authError branch 4 times, most recently from feaff6d to 61a6fc0 Compare September 5, 2023 18:45
@dark0dave
Copy link
Collaborator Author

@stepanstipl ok, reviewed all comments and updated should be more go like apologies.

@dark0dave dark0dave force-pushed the feature/authError branch 3 times, most recently from 3f4e824 to ac6a888 Compare September 5, 2023 23:42
Copy link
Contributor

@stepanstipl stepanstipl left a comment

Choose a reason for hiding this comment

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

I think this is good, just 2 minor nitpicks in code.

But I realised one thing - when looking at the tests. I think the test is good, but I was also thinking it might be good to cover the two scenarios from #450:
a) Nonexisting/wrong cluster endpoint
b) Malformed kubeconfig

And I realised that we probably only cover half of the issue. The 2nd b) failure does not seem to come from getCollectors, but rather from the initCollectors:

	initCollectors := initCollectors(config)

Happy to leave the init bit for another PR, and we probably should for the sake of getting this in, but just to be aware we're fixing just half of the scenarios from #450.

cmd/kubent/main.go Outdated Show resolved Hide resolved
cmd/kubent/main_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@stepanstipl stepanstipl left a comment

Choose a reason for hiding this comment

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

Hi @dark0dave, you want to get this in?

I'm happy with the code 👍, there's just one test failing (I believe it's just the return code 200 -> 100). Cheers 🎉

@dark0dave
Copy link
Collaborator Author

@stepanstipl should be all good now

…og level for silent resource failures

Signed-off-by: dark0dave <[email protected]>
@stepanstipl stepanstipl changed the title feat: Fix auth error fix: Return error exit code when collectors fail Sep 8, 2023
@stepanstipl stepanstipl merged commit 4fa4920 into master Sep 8, 2023
25 checks passed
@stepanstipl stepanstipl deleted the feature/authError branch September 8, 2023 15:07
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.

Return code when the cluster is not accessible or if is there an issue with authentication
2 participants