-
Notifications
You must be signed in to change notification settings - Fork 105
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
OCPBUGS-45421: Linter fixes #1253
OCPBUGS-45421: Linter fixes #1253
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jmencak The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Other relevant PRs: |
Golangci-linter configuration ".ci-operator.yaml" was configured in a way which was showing a lot of false positives and also missing a lot of issues. Fix both the linter configuration and the missing issues that found its way into the codebase.
75a8a5b
to
2fc45c3
Compare
ci/prow/e2e-gcp-pao-updating-profile failed due to infra issues |
The |
|
Tests in 7_performance_kubelet_node/kubelet.go are broken and need fixing, keep the current semantics and make the linter pass.
@jmencak: This pull request references Jira Issue OCPBUGS-45421, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira ([email protected]), skipping review request. The bug has been updated to refer to the pull request using the external bug tracker. In 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. |
/cc @mrniranjan |
@jmencak: The following test failed, say
Full PR test history. Your PR dashboard. 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. |
This PR is ready for review, |
/cc @rbaturov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
looks OK to me
testutils.CustomBeforeAll(func() { | ||
var err error | ||
|
||
workerRTNodes, err = nodes.GetByLabels(testutils.NodeSelectorLabels) | ||
// TODO: The code here is broken as it masks workerRTNodes defined above | ||
// and the value of workerRTNodes is never used. Keep the linter happy for now | ||
// and remove the ineffectual assignment of workerRTNodes, err = nodes.MatchingOptionalSelector(workerRTNodes) below. | ||
workerRTNodes, err := nodes.GetByLabels(testutils.NodeSelectorLabels) | ||
Expect(err).ToNot(HaveOccurred()) | ||
|
||
workerRTNodes, err = nodes.MatchingOptionalSelector(workerRTNodes) | ||
_, err = nodes.MatchingOptionalSelector(workerRTNodes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this means we need a deep review of this suite. Let's put an AI on ourselves
/hold to let others review |
ginkgo.By("cluster changes rollback") | ||
if node != nil { | ||
util.ExecAndLogCommand("oc", "label", "node", "--overwrite", node.Name, nodeLabelRealtime+"-") | ||
_, _, _ = util.ExecAndLogCommand("oc", "label", "node", "--overwrite", node.Name, nodeLabelRealtime+"-") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the linter cannot ignore these at all ? I hope this will not complicated automated cherry picks too much
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can exclude these checks by something like:
- linters:
- errcheck
text: "Error return value of .* is not checked"
however, I'm not sure I'm in favour of doing that as it will mask a lot of cases where the checking is warranted. This PR includes so many fixes and changes that the backports will be difficult anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, in the future, I believe it is worth adding even more linters. Look at the benefit so far -- we might catch more things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok . lets align with the linter
configv1.AddToScheme(scheme.Scheme) | ||
mcov1.AddToScheme(scheme.Scheme) | ||
tunedv1.AddToScheme(scheme.Scheme) | ||
utilruntime.Must(performancev2.AddToScheme(scheme.Scheme)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what was the case here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pkg/performanceprofile/controller/performanceprofile_controller_suite_test.go:21:27: Error return value is not checked (errcheck)
performancev2.AddToScheme(scheme.Scheme)
/lgtm |
/hold cancel |
d24467f
into
openshift:master
@jmencak: Jira Issue OCPBUGS-45421: Some pull requests linked via external trackers have merged: The following pull requests linked via external trackers have not merged: These pull request must merge or be unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with Jira Issue OCPBUGS-45421 has not been moved to the MODIFIED state. In 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. |
[ART PR BUILD NOTIFIER] Distgit: cluster-node-tuning-operator |
Golangci-linter configuration ".ci-operator.yaml" was configured in a way which was showing a lot of false positives and also missing a lot of issues.
Fix both the linter configuration and the missing issues that found its way into the codebase.