Skip to content

Replace logging.SlogNopHandler by slog.DiscardHandler #37847

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 27, 2025
Merged

Replace logging.SlogNopHandler by slog.DiscardHandler #37847

merged 1 commit into from
Feb 27, 2025

Conversation

tklauser
Copy link
Member

The log/slog Go standard library package provides DiscardHandler since Go 1.241. Make use of it instead of duplicating it as logging.SlogNopHandler.

Footnotes

  1. https://go.dev/doc/go1.24#logslogpkglogslog ↩

@tklauser tklauser added kind/cleanup This includes no functional changes. release-note/misc This PR makes changes that have no direct user impact. labels Feb 25, 2025
@tklauser tklauser requested review from a team as code owners February 25, 2025 10:05
@tklauser tklauser force-pushed the pr/tklauser/slog-discard-handler branch from dfddb6d to 2feeb5d Compare February 25, 2025 10:05
@tklauser tklauser requested review from a team as code owners February 25, 2025 10:05
@tklauser
Copy link
Member Author

/test

@tklauser tklauser enabled auto-merge February 25, 2025 10:06
@tklauser tklauser force-pushed the pr/tklauser/slog-discard-handler branch from 2feeb5d to 45d4b33 Compare February 25, 2025 10:23
@tklauser tklauser requested a review from a team as a code owner February 25, 2025 10:23
@tklauser tklauser requested a review from marseel February 25, 2025 10:24
@tklauser
Copy link
Member Author

Pushed an additional commit to bump Go to 1.24 in go.mod so we can actually use Go 1.24 features (otherwise linters will complain).

@tklauser
Copy link
Member Author

/test

@bimmlerd
Copy link
Member

bimmlerd commented Feb 25, 2025 •
edited
Loading

Pushed an additional commit to bump Go to 1.24 in go.mod so we can actually use Go 1.24 features (otherwise linters will complain).

Ah. Won't this break the local build for folks whose distributions haven't caught up to 1.24? EDIT: It will not - just tried it; the go toolchain stuff works! :)

@tklauser
Copy link
Member Author

Looks like updating Go to 1.24 in go.mod leads to some other test fallout, e.g.

--- FAIL: TestSpireDelegateClient_GetCertificateForIdentity (0.00s)

    certificate_provider_test.go:279: failed to generate leaf key: crypto/rsa: 512-bit keys are insecure (see https://go.dev/pkg/crypto/rsa#hdr-Minimum_key_size)

from https://github.com/cilium/cilium/actions/runs/13518990036/job/37774055591.

Will move this PR to draft for the time being.

@tklauser tklauser marked this pull request as draft February 25, 2025 10:58
auto-merge was automatically disabled February 25, 2025 10:58

Pull request was converted to draft

@tklauser tklauser force-pushed the pr/tklauser/slog-discard-handler branch from 45d4b33 to c591612 Compare February 25, 2025 11:21
@tklauser
Copy link
Member Author

/test

@tklauser tklauser mentioned this pull request Feb 25, 2025
Merged
@tklauser
Copy link
Member Author

Split the Go update and corresponding test fixes into a separate PR for easier review: #37852

@tklauser tklauser force-pushed the pr/tklauser/slog-discard-handler branch from c591612 to 48ed649 Compare February 25, 2025 12:15
sayboras
sayboras approved these changes Feb 25, 2025
View reviewed changes
Copy link
Member

@sayboras sayboras left a comment •
edited
Loading

Choose a reason for hiding this comment

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

it's happening go.go go go go go ... (24 times)

@tklauser tklauser force-pushed the pr/tklauser/slog-discard-handler branch from 48ed649 to ca31c79 Compare February 26, 2025 12:44
@tklauser tklauser marked this pull request as ready for review February 26, 2025 12:44
@tklauser
Copy link
Member Author

/test

@tklauser tklauser enabled auto-merge February 26, 2025 12:45
@tklauser tklauser removed request for a team, nathanjsweet and marseel February 26, 2025 12:45
derailed
derailed approved these changes Feb 26, 2025
View reviewed changes
Copy link
Contributor

@derailed derailed left a comment

Choose a reason for hiding this comment

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

@tklauser Nice! Thanks for this update.

@tklauser tklauser added this pull request to the merge queue Feb 26, 2025
@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 Feb 26, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 26, 2025
@tklauser tklauser added this pull request to the merge queue Feb 26, 2025
@tklauser tklauser removed this pull request from the merge queue due to a manual request Feb 26, 2025
@tklauser
Replace logging.SlogNopHandler by slog.DiscardHandler
39da804 
The log/slog Go standard library package provides DiscardHandler since
Go 1.24[^1]. Make use of it instead of duplicating it as
logging.SlogNopHandler.

[^1]: https://go.dev/doc/go1.24#logslogpkglogslog

Signed-off-by: Tobias Klauser <tobias@cilium.io>
@tklauser tklauser force-pushed the pr/tklauser/slog-discard-handler branch from ca31c79 to 39da804 Compare February 26, 2025 22:40
@tklauser tklauser removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Feb 26, 2025
@tklauser
Copy link
Member Author

/test

@tklauser tklauser enabled auto-merge February 26, 2025 22:40
@tklauser tklauser added this pull request to the merge queue Feb 27, 2025
Merged via the queue into main with commit 1084432 Feb 27, 2025
281 checks passed
@tklauser tklauser deleted the pr/tklauser/slog-discard-handler branch February 27, 2025 06:37
@maintainer-s-little-helper maintainer-s-little-helper bot added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Feb 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derailed derailed derailed approved these changes

@joamaki joamaki joamaki approved these changes

@aspsk aspsk aspsk approved these changes

@bimmlerd bimmlerd bimmlerd approved these changes

@sayboras sayboras sayboras approved these changes

Assignees
No one assigned
Labels
kind/cleanup This includes no functional changes. 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tklauser @bimmlerd @derailed @joamaki @aspsk @sayboras