SREP-4482: Update golangci-lint configuration with enhanced linters#716
Conversation
Enhance the golangci-lint configuration to include a more comprehensive set of linters organized by priority (Critical, High, Medium, Optional) with appropriate settings for error handling, security, and code quality checks. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@devppratik: This pull request references SREP-4482 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/label do-not-merge |
|
@devppratik: The label(s) DetailsIn response to this:
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. |
|
/label do-not-merge/hold |
|
@devppratik: The label(s) DetailsIn response to this:
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. |
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
| - examples/ | ||
|
|
||
| run: | ||
| timeout: 5m |
There was a problem hiding this comment.
What is the motivation for removing the concurrency and setting a 5m timeout?
There was a problem hiding this comment.
From https://golangci-lint.run/docs/configuration/file/#run-configuration
In golangci-lint v2, the default concurrency is 0, which automatically matches the Linux container CPU quota and falls back to the number of logical CPUs. Removing the explicit concurrency: 10 setting allows golangci-lint to auto-tune based on available resources, which is the recommended approach.
I have added the timeout setting to prevents indefinite run. It seemed to work without any issues in the local. Can adjust as needed
There was a problem hiding this comment.
Gotcha, so this is a global config meaning each operator will use it. If we run into a project taking too long, we will need to tune this timeout.
Can you test this config on some other projects, like osdctl etc to just get a bit more data to prove the 5m is enough? I have seen timeouts in CI for linting due to resource pressure in the CI clusters.
There was a problem hiding this comment.
While I personally prefer having the CI or surrounding framework (like prek or pre-commit) handle the per-task timeout, 5 minutes seems like a reasonable starting point to build off of, and testing on others like osdctl seems like a reasonable request to get a multi-project baseline.
Other thought: we should fail builds if this times out so we can get that feedback and make the correction.
There was a problem hiding this comment.
@joshbranham Yes, this is more of a global config that we add. We can look. to have this fine tune later per project if required. I did check with osdctl locally and it seems to be running without issues.
There was a problem hiding this comment.
Ran this on a couple other projects and measured time in seconds so 5m should have plenty of buffer. LGTM
devppratik
changed the title
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/hold Putting on hold until APAC Time to merge and update operators |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bmeng, devppratik The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
6 participants
Based on the agreed golden standard for golangci.yml which can be used by all the agents & developers, enhance the golangci-lint configuration to include a more comprehensive set of linters organized by priority (Critical, High, Medium, Optional) with appropriate settings for error handling, security, and code quality checks.
Related to SREP-4482