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

ci(deps): upgrade golangci-lint #2556

Merged
merged 40 commits into from
Jul 29, 2024
Merged

Conversation

jkroepke
Copy link
Contributor

@jkroepke jkroepke commented Jul 21, 2024

What type of PR is this?

cleanup

Which issue does this PR fix:

Cut off from #2532

What does this PR do / Why do we need it:

I had issues with the old golangci-lint version, see https://github.com/project-zot/zot/actions/runs/10028369912/job/27715210368?pr=2532

ref: golangci/golangci-lint#3718

If an issue # is not available please add repro steps and logs showing the issue:

Testing done on this change:

Automation added to e2e:

Will this break upgrades or downgrades?

Does this PR introduce any user-facing change?:


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link

codecov bot commented Jul 21, 2024

Codecov Report

Attention: Patch coverage is 97.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 92.72%. Comparing base (7d87558) to head (165feba).

Files Patch % Lines
pkg/meta/dynamodb/iterator.go 66.66% 1 Missing ⚠️
pkg/storage/imagestore/imagestore.go 66.66% 1 Missing ⚠️
pkg/test/common/fs.go 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2556      +/-   ##
==========================================
- Coverage   92.72%   92.72%   -0.01%     
==========================================
  Files         169      169              
  Lines       22461    22463       +2     
==========================================
  Hits        20828    20828              
- Misses       1016     1018       +2     
  Partials      617      617              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@andaaron andaaron left a comment

Choose a reason for hiding this comment

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

Thank you. @jkroepke for the PR. I do have some questions on the disabled linters.

golangcilint.yaml Outdated Show resolved Hide resolved
golangcilint.yaml Outdated Show resolved Hide resolved
golangcilint.yaml Outdated Show resolved Hide resolved
golangcilint.yaml Outdated Show resolved Hide resolved
golangcilint.yaml Outdated Show resolved Hide resolved
golangcilint.yaml Outdated Show resolved Hide resolved
golangcilint.yaml Outdated Show resolved Hide resolved
golangcilint.yaml Outdated Show resolved Hide resolved
golangcilint.yaml Outdated Show resolved Hide resolved
golangcilint.yaml Outdated Show resolved Hide resolved
Signed-off-by: Jan-Otto Kröpke <mail@jkroepke.de>
Signed-off-by: Jan-Otto Kröpke <mail@jkroepke.de>
@jkroepke
Copy link
Contributor Author

I enabled the mentioned linters. The results should be visible in the UI.

lint settings are very moody. From my point of view, the motivation to go over all lints, fix the lint issue or ignore should be more decided by an zot maintainer.

Some linter are supporting auto-fix which introduced a lot of white spaces changes and resulting into merge conflicts again.

There are 2 possibilities to move forward:

  • 1 big PR which covers all maintainer needs.
  • 1 big PR which does to golang-ci update with disabled lint settings and multiple small separate PR for each dedicated disabled linter.

@andaaron
Copy link
Contributor

Thank you @jkroepke for looking into this.

@ram, based on the latest lint output I think we can:

  • promlinter - add to the ignore list, the impact of the renames is not clear to me
  • protogetter - we should update the code to fix the issues, there are some valuable checks for nil pointers in the getters we don't use
  • canonicalheader - we should update the code to fix the issues, Docker-Content-Digest should be fixed, I am not sure of the implications of it being capitalized wrong right now.
  • contextcheck - I think we should use nolint in these specific cases, I don't understand why we would use a context in the cases it highlight.
  • copyloopvar - add to the ignore list or update the code to fix, theoretically the code would perform better if we don't copy the variables.
  • mnd - update the nolint messages to replace gomnd with mnd, I also think we can use nolint in the new places it found
  • nilnil - use nolint, I think we are justified to keep the code logic as is in our use case for scheduler
  • perfsprint - I think we can update the code to fix these, what do you think @ram? Theoretically there should be some performance improvement.
  • wsl - we should update the code to fix these
  • zerologlint - this is a false positive, we should use nolint

@jkroepke
Copy link
Contributor Author

contextcheck - I think we should use nolint in these specific cases, I don't understand why we would use a context in the cases it highlight.

One use-case is to pass the context from end-to-end. For example, if a request get cancels, it would also cancel all outstanding operations, too.

For perfsprint and wsl, there is an auto fix command. Do not do it by hand.

Signed-off-by: Jan-Otto Kröpke <joe@cloudeteer.de>
Signed-off-by: Jan-Otto Kröpke <joe@cloudeteer.de>
…-append -test=true -fix ./...

Signed-off-by: Jan-Otto Kröpke <joe@cloudeteer.de>
Signed-off-by: Jan-Otto Kröpke <joe@cloudeteer.de>
Signed-off-by: Jan-Otto Kröpke <joe@cloudeteer.de>
Signed-off-by: Jan-Otto Kröpke <joe@cloudeteer.de>
@andaaron
Copy link
Contributor

Why are the imports changed? Looks like these settings are not respected?

  gci:
    sections:
      - standard
      - default
      - prefix(zotregistry.dev/zot)     

The default and prefix imports are not separated in the last update.

…ite ."

This reverts commit 5bf8c42.

Signed-off-by: Jan-Otto Kröpke <joe@cloudeteer.de>
…d' -s default -s 'prefix(zotregistry.dev/zot)' .

Signed-off-by: Jan-Otto Kröpke <joe@cloudeteer.de>
Signed-off-by: Jan-Otto Kröpke <joe@cloudeteer.de>
@jkroepke
Copy link
Contributor Author

Make me sad that test failing here because of running lint fixes.

Are you interest to continue? Going through all lint issues and resolve creates a hugh amount of line diff with endless merge conflicts.

I still recommend disable some linter and resolve the issues in a upcoming PR.

@rchincha
Copy link
Contributor

Are you interest to continue? Going through all lint issues and resolve creates a hugh amount of line diff with endless merge conflicts.

@jkroepke fully familiar with this situation 😞 having done this several times. And your effort here is much appreciated. Let me take a look at the list a little more carefully so we can find a good compromise.

Signed-off-by: Jan-Otto Kröpke <joe@cloudeteer.de>
Signed-off-by: Jan-Otto Kröpke <joe@cloudeteer.de>
Signed-off-by: Jan-Otto Kröpke <joe@cloudeteer.de>
Signed-off-by: Jan-Otto Kröpke <joe@cloudeteer.de>
@jkroepke
Copy link
Contributor Author

wsl issues are crazy high, since the test files seems like copy/paste and the same issues appeared over and over again.

But it looks good for now. Only the benchmark tests reports that something is way slower. But I had the same issue, in #2532 too

@andaaron Sorry I do not see your PR. But feel free to merge and I'm going to the hell trough conflicts. It's okay

Signed-off-by: Jan-Otto Kröpke <joe@cloudeteer.de>
@jkroepke jkroepke requested a review from andaaron July 25, 2024 11:13
Signed-off-by: Jan-Otto Kröpke <joe@cloudeteer.de>
Signed-off-by: Jan-Otto Kröpke <joe@cloudeteer.de>
Signed-off-by: Jan-Otto Kröpke <joe@cloudeteer.de>
@andaaron
Copy link
Contributor

wsl issues are crazy high, since the test files seems like copy/paste and the same issues appeared over and over again.

But it looks good for now. Only the benchmark tests reports that something is way slower. But I had the same issue, in #2532 too

@andaaron Sorry I do not see your PR. But feel free to merge and I'm going to the hell trough conflicts. It's okay

My PR is linked to this one, #2571, but not merged yet. No updates since yesterday, waiting on the review from @rchincha.

I reviewed all the changes in this PR so far and it looks good.

Signed-off-by: Jan-Otto Kröpke <joe@cloudeteer.de>
Signed-off-by: Jan-Otto Kröpke <joe@cloudeteer.de>
Signed-off-by: Jan-Otto Kröpke <joe@cloudeteer.de>
Signed-off-by: Jan-Otto Kröpke <joe@cloudeteer.de>
Signed-off-by: Jan-Otto Kröpke <joe@cloudeteer.de>
Signed-off-by: Jan-Otto Kröpke <joe@cloudeteer.de>
Signed-off-by: Jan-Otto Kröpke <joe@cloudeteer.de>
rchincha pushed a commit that referenced this pull request Jul 25, 2024
Originally found by @jkroepke in: https://github.com/project-zot/zot/actions/runs/10056774230/job/27796324933?pr=2556
And #2556 in general

This commit covers:
- (canonicalheader) capitalization of Docker-Content-Digest header
- (protogetter) the proto getters were not used, they check for nil pointers, we should switch to using them
- (zerologlint) fix the false positive

Signed-off-by: Andrei Aaron <aaaron@luxoft.com>
@andaaron
Copy link
Contributor

@andaaron Sorry I do not see your PR. But feel free to merge and I'm going to the hell trough conflicts. It's okay

My PR was merged.

@andaaron
Copy link
Contributor

The Clustering test / Stateless zot with shared reliable storage (pull_request) job should be fixed now.

Signed-off-by: Jan-Otto Kröpke <joe@cloudeteer.de>
Signed-off-by: Jan-Otto Kröpke <joe@cloudeteer.de>
@jkroepke
Copy link
Contributor Author

The golangci-lint check is green 🎉

To include #2571, I do a merge from main.

Signed-off-by: Jan-Otto Kröpke <joe@cloudeteer.de>
Copy link
Contributor

@andaaron andaaron left a comment

Choose a reason for hiding this comment

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

Looks good!

@rchincha rchincha merged commit f618b1d into project-zot:main Jul 29, 2024
37 of 39 checks passed
@rchincha
Copy link
Contributor

@jkroepke thanks for getting this done. These types of tasks are very painful but clear the inevitable accumulation of tech debt!

@jkroepke jkroepke deleted the upgrade/golang-ci branch July 31, 2024 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants