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

choir: normalize error handling in kube_proxy_replacement.go #16811

Merged
merged 1 commit into from Jul 9, 2021

Conversation

ldelossa
Copy link
Contributor

@ldelossa ldelossa commented Jul 6, 2021

this commit normalizes the error handling in kube_proxy_replacement.go
by bubbling errors up to the caller of the initialization method.

Signed-off-by: Louis DeLosSantos louis.delos@isovalent.com

@ldelossa ldelossa requested a review from a team July 6, 2021 19:01
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 6, 2021
@ldelossa ldelossa requested a review from joamaki July 6, 2021 19:01
@ldelossa
Copy link
Contributor Author

ldelossa commented Jul 6, 2021

cc @joestringer @christarazi

will be more of these per our discussion on normalizing the error handling for depedencies in NewDaemon.

@ldelossa ldelossa force-pushed the louis/normalize-err-handling branch from a1d4d7b to 2f12813 Compare July 6, 2021 19:04
@ldelossa ldelossa marked this pull request as draft July 6, 2021 19:06
@joestringer joestringer added the release-note/misc This PR makes changes that have no direct user impact. label Jul 6, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 6, 2021
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Couple of super minor nits, otherwise LGTM.

daemon/cmd/kube_proxy_replacement.go Show resolved Hide resolved
daemon/cmd/kube_proxy_replacement.go Show resolved Hide resolved
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

LGTM, thanks. One note in addition to Joe's comments

daemon/cmd/kube_proxy_replacement.go Outdated Show resolved Hide resolved
@ldelossa ldelossa force-pushed the louis/normalize-err-handling branch from 2f12813 to c86c3f3 Compare July 6, 2021 21:05
@ldelossa
Copy link
Contributor Author

ldelossa commented Jul 6, 2021

@christarazi @joestringer
nits addressed.

Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

Looks like we have some legit failures in the go linting checkers in addition to the below:

pkg/nodediscovery/nodediscovery.go Outdated Show resolved Hide resolved
pkg/nodediscovery/nodediscovery.go Outdated Show resolved Hide resolved
@ldelossa ldelossa force-pushed the louis/normalize-err-handling branch from c86c3f3 to 6ff6618 Compare July 7, 2021 13:52
@ldelossa ldelossa marked this pull request as ready for review July 7, 2021 13:52
@ldelossa
Copy link
Contributor Author

ldelossa commented Jul 7, 2021

@christarazi @joestringer sorry about that bit of confusion, that nodediscovery.go file was not supposed to be in this PR at all. Removed it from the edited set and force pushed.

Copy link
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

LGTM only one nit that doesn't require my 2nd review

@@ -41,12 +41,12 @@ import (
"golang.org/x/sys/unix"
)

func initKubeProxyReplacementOptions() (strict bool) {
func initKubeProxyReplacementOptions() (bool, error) {
Copy link
Member

Choose a reason for hiding this comment

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

hint to leave a comment about the returned values since now we don't know what is bool used for.

Copy link
Contributor Author

@ldelossa ldelossa Jul 8, 2021

Choose a reason for hiding this comment

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

@aanm I updated this function with a comment describing its usage, can you please read this comment and ensure I have its usage summarized correctly.

this commit normalizes the error handling in kube_proxy_replacement.go
by bubbling errors up to the caller of the initialization method.

Signed-off-by: Louis DeLosSantos <louis.delos@isovalent.com>
@ldelossa ldelossa force-pushed the louis/normalize-err-handling branch from 6ff6618 to 16c502a Compare July 8, 2021 13:43
@ldelossa ldelossa added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 8, 2021
@kkourt kkourt merged commit db5300d into cilium:master Jul 9, 2021
@ldelossa ldelossa deleted the louis/normalize-err-handling branch July 9, 2021 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants