Skip to content

chore: Migrate repository to golangci-lint v2#811

Merged
openshift-merge-bot[bot] merged 1 commit into
openshift:mainfrom
MateSaary:migrate-golangci-lint-v2
Oct 24, 2025
Merged

chore: Migrate repository to golangci-lint v2#811
openshift-merge-bot[bot] merged 1 commit into
openshift:mainfrom
MateSaary:migrate-golangci-lint-v2

Conversation

@MateSaary

@MateSaary MateSaary commented Oct 22, 2025

Copy link
Copy Markdown
Member

What type of PR is this?

  • fix (Bug Fix)
  • feat (New Feature)
  • docs (Documentation)
  • test (Test Coverage)
  • chore (Clean Up / Maintenance Tasks)
  • other (Anything that doesn't fit the above)

What this PR does / Why we need it?

This PR migrates golangci-lint from version 1.61 to the latest, 2.5. Due to the stricter linting introduced (particularly with errcheck), quite a few updates to existing code was needed.

Not sure if we generally like this change appearance/readability wise but it does make code more semantically correct/less bug-prone, happy to discuss.

Which Jira/Github issue(s) does this PR fix?

Special notes for your reviewer

Unit Test Coverage

Guidelines

  • If it's a new sub-command or new function to an existing sub-command, please cover at least 50% of the code
  • If it's a bug fix for an existing sub-command, please cover 70% of the code

Test coverage checks

  • Added unit tests
  • Created jira card to add unit test
  • This PR may not need unit tests

Pre-checks (if applicable)

  • Ran unit tests locally
  • Validated the changes in a cluster
  • Included documentation changes with PR
  • Backward compatible

/label tide/merge-method-squash

@openshift-ci openshift-ci Bot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Oct 22, 2025
@codecov-commenter

codecov-commenter commented Oct 22, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 68.75000% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 53.24%. Comparing base (a0af80a) to head (8f9b39d).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
cmd/ocm-backplane/cloud/common.go 62.50% 5 Missing and 1 partial ⚠️
internal/upgrade/upgrade.go 0.00% 3 Missing ⚠️
pkg/accessrequest/accessRequest.go 0.00% 3 Missing ⚠️
pkg/container/container_podman.go 0.00% 2 Missing ⚠️
pkg/ocm/ocm.go 0.00% 2 Missing ⚠️
pkg/cli/session/session.go 80.00% 1 Missing ⚠️
pkg/container/container_docker.go 0.00% 1 Missing ⚠️
pkg/elevate/elevate.go 50.00% 1 Missing ⚠️
pkg/utils/util.go 75.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #811      +/-   ##
==========================================
+ Coverage   52.83%   53.24%   +0.40%     
==========================================
  Files          86       86              
  Lines        6420     6491      +71     
==========================================
+ Hits         3392     3456      +64     
- Misses       2577     2581       +4     
- Partials      451      454       +3     
Files with missing lines Coverage Δ
cmd/ocm-backplane/cloud/ssm.go 56.08% <100.00%> (ø)
cmd/ocm-backplane/managedJob/createManagedJob.go 67.47% <100.00%> (ø)
cmd/ocm-backplane/testJob/createTestJob.go 74.47% <100.00%> (ø)
internal/github/github.go 68.36% <100.00%> (ø)
pkg/ai/mcp/backplane_cluster_resource.go 42.10% <100.00%> (ø)
pkg/awsutil/sts.go 63.07% <100.00%> (ø)
pkg/login/additional_login_detector.go 90.90% <100.00%> (ø)
pkg/login/kubeConfig.go 68.64% <100.00%> (ø)
pkg/monitoring/monitoring.go 39.47% <100.00%> (ø)
pkg/utils/renderingutils.go 58.20% <100.00%> (ø)
... and 9 more

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@MateSaary MateSaary force-pushed the migrate-golangci-lint-v2 branch 3 times, most recently from 5c041d0 to 4b254c9 Compare October 22, 2025 20:13
@MateSaary MateSaary force-pushed the migrate-golangci-lint-v2 branch from 4b254c9 to 8f9b39d Compare October 22, 2025 20:40
Comment thread .golangci.yml
- staticcheck
- typecheck
- unused
- stylecheck

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It seems we are removing stylecheck. Is v2 not supporting this or any other reason?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yep, stylecheck has been merged into staticcheck 🙂 (also a few others) source: https://golangci-lint.run/docs/product/migration-guide/#lintersenablestylecheckgosimplestaticcheck

Comment thread Makefile
# Installed using instructions from: https://golangci-lint.run/usage/install/#linux-and-windows
getlint:
@mkdir -p $(GOPATH)/bin
@ls $(GOPATH)/bin/golangci-lint 1>/dev/null || (echo "Installing golangci-lint..." && curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(GOPATH)/bin $(GOLANGCI_LINT_VERSION))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you elaborate on why we are removing the ls check here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The main reason for the change here is to ensure golangci-lint is reinstalled properly each time, I think ls itself wouldn't be an issue but our CI did keep failing until I forced it to overwrite the existing installation to ensure the latest version is installed.

Comment thread pkg/cli/session/session.go
@feichashao

Copy link
Copy Markdown
Contributor

@MateSaary thank you for the PR to improve the code quality!

I left 2 comments inline regarding the changes on the lint config and makefile, could you help to elaborate on them?

Notes to other reviewers: don't be scared by the number of files changed, they are mostly add a _ to the return to make linter happy.

@MateSaary

Copy link
Copy Markdown
Member Author

@feichashao thank you for the review and comments, hopefully my responses help answer your questions 🙂 Let me know if there's any further questions or clarification needed!

@feichashao

Copy link
Copy Markdown
Contributor

/lgtm
/approve

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Oct 24, 2025
@openshift-ci

openshift-ci Bot commented Oct 24, 2025

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feichashao, MateSaary

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 24, 2025
@openshift-ci

openshift-ci Bot commented Oct 24, 2025

Copy link
Copy Markdown
Contributor

@MateSaary: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@openshift-merge-bot openshift-merge-bot Bot merged commit bd0c59c into openshift:main Oct 24, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants