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.

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

add slowg analyzer to check inappropriate use of log/slog #2

Merged
merged 2 commits into from
Jul 18, 2023
Merged

add slowg analyzer to check inappropriate use of log/slog #2

merged 2 commits into from
Jul 18, 2023

Conversation

rolinh
Copy link
Member

@rolinh rolinh commented Jul 11, 2023

No description provided.

@rolinh rolinh force-pushed the pr/rolinh/slowg branch 3 times, most recently from 77e68cf to 48b6697 Compare July 11, 2023 14:51
@rolinh rolinh mentioned this pull request Jul 11, 2023
Merged
@rolinh rolinh force-pushed the pr/rolinh/slowg branch 2 times, most recently from ce4b95e to 6be8fd4 Compare July 12, 2023 16:32
@chancez chancez changed the title add slowg analyzer to check inappropriate use of log/slog add slog analyzer to check inappropriate use of log/slog Jul 12, 2023
@chancez chancez changed the title add slog analyzer to check inappropriate use of log/slog add slowg analyzer to check inappropriate use of log/slog Jul 12, 2023
nathanjsweet
nathanjsweet requested changes Jul 12, 2023
View reviewed changes
Copy link
Member

@nathanjsweet nathanjsweet 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.

Will this analyzer run by default? If so, I'm worried that it will break the cilium repo.
Everything else looks good.

@rolinh
Copy link
Member Author

rolinh commented Jul 12, 2023

Will this analyzer run by default?

I've enabled it by default, yes.

If so, I'm worried that it will break the cilium repo.

In which way? I don't understand your concern as Cilium is not even using log/slog yet 🤔

@rolinh rolinh force-pushed the pr/rolinh/slowg branch from 6be8fd4 to b5b262d Compare July 13, 2023 07:58
@rolinh rolinh marked this pull request as draft July 14, 2023 07:16
@rolinh
Copy link
Member Author

rolinh commented Jul 14, 2023 •
edited
Loading

Converted back to draft, the log functions were renamed from XXXCtx to XXXContext and thus the linter needs an update (see golang/go#61200).

@rolinh rolinh force-pushed the pr/rolinh/slowg branch from b5b262d to 510bbf2 Compare July 14, 2023 08:06
@rolinh
Copy link
Member Author

rolinh commented Jul 14, 2023

CI is now failing as there is no tagged release of Go 1.21 that includes this change yet. Unfortunately, it looks like we cannot specify tip or a commit sha as the Go version to use in the matrix of tests.

@rolinh
Copy link
Member Author

rolinh commented Jul 14, 2023

I'll mark the PR as ready to merge. CI should turn green once we update Go 1.21 to the RC.3.

@rolinh rolinh marked this pull request as ready for review July 14, 2023 11:42
@rolinh rolinh requested a review from nathanjsweet July 14, 2023 11:42
kaworu
kaworu approved these changes Jul 14, 2023
View reviewed changes
ioreadall/io_readall.go Outdated Show resolved Hide resolved
@rolinh rolinh force-pushed the pr/rolinh/slowg branch from 510bbf2 to e785664 Compare July 18, 2023 12:11
@rolinh rolinh force-pushed the pr/rolinh/slowg branch from e785664 to 346b369 Compare July 18, 2023 12:14
@rolinh
Copy link
Member Author

rolinh commented Jul 18, 2023

Go v1.21.0-rc.3 is out so I updated the Go matrix in the CI tests and, as expected, the tests now pass.

@nathanjsweet Could you please check this PR again or give a rationale as to why you think we shouldn't merge it as is?

nathanjsweet
nathanjsweet approved these changes Jul 18, 2023
View reviewed changes
Copy link
Member

@nathanjsweet nathanjsweet left a comment

Choose a reason for hiding this comment

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

Sorry, no objections. I saw that you converted it to a draft at one point and I forgot to pick it back up. Very cool!

@nathanjsweet nathanjsweet merged commit 2a3819b into main Jul 18, 2023
@nathanjsweet nathanjsweet deleted the pr/rolinh/slowg branch July 18, 2023 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nathanjsweet nathanjsweet nathanjsweet approved these changes

@kaworu kaworu kaworu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rolinh @kaworu @nathanjsweet