Skip to content

Conversation

giorio94
Copy link
Member

@giorio94 giorio94 commented Oct 15, 2024

Extend the log attributes mangling logic to ensure consistency in the attribute key used to identify an error. Indeed, the logr.Error method takes an error as first parameter, and the slog adapter converts it into an attribute identified as err. Let's replace that into error to ensure consistency with all the other occurrences.

@giorio94 giorio94 added the release-note/misc This PR makes changes that have no direct user impact. label Oct 15, 2024
@giorio94 giorio94 requested a review from a team as a code owner October 15, 2024 10:03
@giorio94 giorio94 requested a review from mhofstetter October 15, 2024 10:03
@giorio94
Copy link
Member Author

/test

Copy link
Member

@mhofstetter mhofstetter left a comment

Choose a reason for hiding this comment

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

Thanks! Only two questions regarding the naming of the "replace"-functions. Maybe it's worth to adjust them to something more generic? (replaceFuncWithoutTimestamp & replaceFunc ?)

Slightly generalize the log attributes mangling logic in preparation for
the subsequent extension, changing the function names to be agnostic of
the specific operations and making one rely on the other, to ensure
consistency and reduce code duplication.

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
Extend the log attributes mangling logic to ensure consistency in the
attribute key used to identify an error. Indeed, the logr.Error method
takes an error as first parameter, and the slog adapter converts it into
an attribute identified as `err`. Let's replace that into `error` to
ensure consistency with all the other occurrences.

Related: d91e387 ("cec: Switch to slog for CEC")
Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
@giorio94
Copy link
Member Author

/test

Copy link
Member

@mhofstetter mhofstetter 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!

@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 Oct 21, 2024
@sayboras sayboras added this pull request to the merge queue Oct 22, 2024
Merged via the queue into cilium:main with commit 1c634e2 Oct 22, 2024
65 checks passed
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.

3 participants