Skip to content

Conversation

sayboras
Copy link
Member

@sayboras sayboras commented Nov 2, 2024

As highlighted in below comment, the default logger is completely replaced (rather than being mutated) during the configuration phase, so we should avoid using global slog instance.

Relates: #35567

Reported-by: Marco Iorio marco.iorio@isovalent.com

@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 Nov 2, 2024
@sayboras sayboras force-pushed the pr/tammach/watcher-logger branch from 54166e1 to 6031f83 Compare November 2, 2024 02:58
@sayboras sayboras added the release-note/misc This PR makes changes that have no direct user impact. label Nov 2, 2024
@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 Nov 2, 2024
@sayboras
Copy link
Member Author

sayboras commented Nov 2, 2024

/test

@sayboras sayboras marked this pull request as ready for review November 4, 2024 10:53
@sayboras sayboras requested review from a team as code owners November 4, 2024 10:53
Copy link
Member

@giorio94 giorio94 left a comment

Choose a reason for hiding this comment

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

Thanks! I looks a bit weird having to pass the logger everywhere, but I agree it seems the least friction path for the moment, without requiring a large refactoring that is definitely out of scope here.

Copy link
Member

@pippolo84 pippolo84 left a comment

Choose a reason for hiding this comment

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

LGTM minus Marco's comments 🚀

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 4, 2024
@pippolo84 pippolo84 removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 4, 2024
@pippolo84
Copy link
Member

Manually removing ready-to-merge label to give @sayboras a chance to address Marco's comments before merging.

@sayboras
Copy link
Member Author

sayboras commented Nov 4, 2024

Manually removing ready-to-merge label to give @sayboras a chance to address Marco's comments before merging.

Thanks a lot 👍

As highlighted in below comment, the default logger is completely
replaced (rather than being mutated) during the configuration phase, so
we should avoid using global slog instance.

Relates: #35567

Reported-by: Marco Iorio <marco.iorio@isovalent.com>
Signed-off-by: Tam Mach <tam.mach@cilium.io>
@sayboras sayboras force-pushed the pr/tammach/watcher-logger branch from 6031f83 to 6be5b67 Compare November 4, 2024 21:54
@sayboras
Copy link
Member Author

sayboras commented Nov 4, 2024

/test

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 4, 2024
@sayboras sayboras added this pull request to the merge queue Nov 4, 2024
Merged via the queue into main with commit 0e4bc5e Nov 4, 2024
269 checks passed
@sayboras sayboras deleted the pr/tammach/watcher-logger branch November 4, 2024 23:46
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.

4 participants