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: update golangci-lint to v1.61.0 #13602

Merged
merged 3 commits into from
Sep 16, 2024

Conversation

blkperl
Copy link
Contributor

@blkperl blkperl commented Sep 14, 2024

Motivation

  • A newer version of golangci-lint is requried to support golang 1.23

Modifications

Changed exportloopref to copyloopvar

Fixed the following lint errors

  • printf: non-constant format string in call to github.com/sirupsen/logrus.Infof
  • printf: non-constant format string in call to (*testing.common).Fatalf (govet)
  • printf: non-constant format string in call to fmt.Sprintf (govet)
  • printf: non-constant format string in call to google.golang.org/grpc/status.Errorf (govet)
  • suite-broken-parallel: testify v1 does not support suite's parallel tests and subtests
  • suite-subtest-run: use s.Run to run subtest (testifylint)
  • The copy of the 'for' variable "limit" can be deleted (Go 1.22+) (copyloopvar)

Verification

Lint passes and existing test coverage

@blkperl blkperl marked this pull request as ready for review September 15, 2024 16:17
@agilgur5 agilgur5 requested a review from Joibel September 15, 2024 19:14
@agilgur5 agilgur5 added type/dependencies PRs and issues specific to updating dependencies go Pull requests that update Go dependencies area/build Build or GithubAction/CI issues labels Sep 15, 2024
@isubasinghe isubasinghe added the prioritized-review For members of the Sustainability Effort label Sep 16, 2024
@isubasinghe
Copy link
Member

I believe this fixes memory usage issues, currently occasionally hit 84gb of memory usage before OOM.

@Joibel
Copy link
Member

Joibel commented Sep 16, 2024

I believe this fixes memory usage issues, currently occasionally hit 84gb of memory usage before OOM.

Which part fixes that?

@Joibel Joibel self-assigned this Sep 16, 2024
@isubasinghe
Copy link
Member

isubasinghe commented Sep 16, 2024

The newer version of golangci-lint

@Joibel
Copy link
Member

Joibel commented Sep 16, 2024

Oh, during linting, not at runtime. More coffee needed. Ok - I remember you saying it did mad things for you.

@@ -43,7 +43,6 @@ func (s *CronSuite) TearDownSuite() {

func (s *CronSuite) TestBasic() {
s.Run("TestBasic", func() {
s.T().Parallel()
Copy link
Member

Choose a reason for hiding this comment

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

These don't do anything, and as this was the only attempt in the code to parallelise tests, we could remove the -parallel flag on the test suite.

I'd like to find a way to reintroduce parallel testing in the future though, so let's not do that.

@Joibel Joibel merged commit a968943 into argoproj:main Sep 16, 2024
27 checks passed
@blkperl blkperl deleted the chore_update_golangci branch September 16, 2024 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build Build or GithubAction/CI issues go Pull requests that update Go dependencies prioritized-review For members of the Sustainability Effort type/dependencies PRs and issues specific to updating dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants