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

chore: normalize returning of errors in NewDaemon #16861

Merged
merged 1 commit into from Jul 15, 2021

Conversation

ldelossa
Copy link
Contributor

house-keeping PR to ensure similar behavior in the NewDaemon method.
After this commit all error conditions are returned if the intent is to
fatal the process.

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

daemon/cmd/daemon.go Outdated Show resolved Hide resolved
@ldelossa ldelossa force-pushed the louis/normalize-err-handling branch from a7bb5ab to d0ad10a Compare July 12, 2021 23:07
@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 12, 2021
@ldelossa
Copy link
Contributor Author

test-me-please

@ldelossa ldelossa changed the title choir: normalize returning of errors in NewDaemon chore: normalize returning of errors in NewDaemon Jul 13, 2021
@ldelossa ldelossa added the release-note/misc This PR makes changes that have no direct user impact. label Jul 13, 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 13, 2021
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.

Changes look good to me. Should we be applying https://github.com/golang/go/wiki/CodeReviewComments#error-strings to the error messages as well?

@ldelossa
Copy link
Contributor Author

@christarazi I'm not opposed to standardizing our error message handling by that, as long as we maintain consistency it looks like a good ruler for us to use.

@ldelossa ldelossa force-pushed the louis/normalize-err-handling branch from d0ad10a to 39575e9 Compare July 13, 2021 17:41
@ldelossa
Copy link
Contributor Author

@christarazi I pre-emptively pushed a commit with all the errors conforming to the prose you provided. If any others don't like this style, feel free to debate it here, I can easily back out the change.

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.

✔️

@ldelossa
Copy link
Contributor Author

test-me-please

house-keeping PR to ensure similar behavior in the NewDaemon method.
After this commit all error conditions are returned if the intent is to
fatal the process.

Signed-off-by: Louis DeLosSantos <louis.delos@isovalent.com>
@ldelossa ldelossa force-pushed the louis/normalize-err-handling branch from 39575e9 to b930eb7 Compare July 14, 2021 18:10
@ldelossa
Copy link
Contributor Author

test-me-please

@ldelossa ldelossa added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 14, 2021
@nebril nebril merged commit feb38cf into cilium:master Jul 15, 2021
@ldelossa ldelossa deleted the louis/normalize-err-handling branch July 15, 2021 12:27
ldelossa added a commit to ldelossa/cilium that referenced this pull request Mar 30, 2022
commit cilium#16861 introduced a normalization of error handling into the
daemon/cmd package.

by doing so we swallowed useful error logs.

this commit adds the error logs back and adds a few additional
fmt.Errorf wrappers where logging is not adequate

Signed-off-by: Louis DeLosSantos <louis.delos@isovalent.com>
joestringer pushed a commit that referenced this pull request Apr 30, 2022
commit #16861 introduced a normalization of error handling into the
daemon/cmd package.

by doing so we swallowed useful error logs.

this commit adds the error logs back and adds a few additional
fmt.Errorf wrappers where logging is not adequate

Signed-off-by: Louis DeLosSantos <louis.delos@isovalent.com>
aditighag pushed a commit to aditighag/cilium that referenced this pull request May 2, 2022
[ upstream commit 6154322 ]

commit cilium#16861 introduced a normalization of error handling into the
daemon/cmd package.

by doing so we swallowed useful error logs.

this commit adds the error logs back and adds a few additional
fmt.Errorf wrappers where logging is not adequate

Signed-off-by: Louis DeLosSantos <louis.delos@isovalent.com>
Signed-off-by: Aditi Ghag <aditi@cilium.io>
aditighag pushed a commit that referenced this pull request May 3, 2022
[ upstream commit 6154322 ]

commit #16861 introduced a normalization of error handling into the
daemon/cmd package.

by doing so we swallowed useful error logs.

this commit adds the error logs back and adds a few additional
fmt.Errorf wrappers where logging is not adequate

Signed-off-by: Louis DeLosSantos <louis.delos@isovalent.com>
Signed-off-by: Aditi Ghag <aditi@cilium.io>
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

5 participants