Remove unneeded loop variable copying #1946
Conversation
k8s-ci-robot
added
release-note-none
|
Hi @alexandear. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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/test-infra repository. |
k8s-ci-robot
added
needs-ok-to-test
✅ Deploy Preview for kubernetes-sigs-kueue ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
@alculquicondor @alexandear I understand that we don't use loop variables in the jobframework, and admissionchecks. But, let us confirm if no issues happen on the external job manager and admission controller implemented by Go 1.21 when we implement concurrent processes without loop variables for the future.
| disabled-checks: # temporarily disabled checks, will fix them later | ||
| - appendAssign | ||
| - assignOp | ||
| - captLocal | ||
| - commentFormatting | ||
| - deprecatedComment | ||
| - elseif | ||
| - exitAfterDefer | ||
| - ifElseChain |
There was a problem hiding this comment.
Without these lines, a lot of the gocritic issues are reported, which I don't want to fix in this PR to avoid mixing things.
Running make ci-lint with removed disabled-checks section
pkg/controller/constants/constants.go:25:2: deprecatedComment: use `Deprecated: ` (note the casing) instead of `DEPRECATED: ` (gocritic)
// DEPRECATED: Use QueueLabel as a label key.
^
pkg/util/resource/resource.go:35:10: elseif: can replace 'else {if cond {}}' with 'else if cond {}' (gocritic)
} else {
^
pkg/util/testing/client.go:143:72: captLocal: `SubResourceName' should not be capitalized (gocritic)
func TreatSSAAsStrategicMerge(ctx context.Context, clnt client.Client, SubResourceName string, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error {
^
pkg/util/testing/wrappers.go:883:49: captLocal: `LocationType' should not be capitalized (gocritic)
func (mkc *MultiKueueClusterWrapper) KubeConfig(LocationType kueuealpha.LocationType, location string) *MultiKueueClusterWrapper {
^
pkg/util/testing/wrappers.go:745:2: commentFormatting: put a space between `//` and comment text (gocritic)
//nothing
^
pkg/util/admissioncheck/admissioncheck.go:118:30: captLocal: `ControllerName' should not be capitalized (gocritic)
func IndexerByConfigFunction(ControllerName string, gvk schema.GroupVersionKind) client.IndexerFunc {
^
pkg/util/admissioncheck/admissioncheck.go:133:100: captLocal: `ControllerName' should not be capitalized (gocritic)
func FilterForController(ctx context.Context, c client.Client, states []kueue.AdmissionCheckState, ControllerName string) ([]string, error) {
^
pkg/controller/admissionchecks/provisioning/controller.go:798:3: assignOp: replace `backoffDuration = backoffDuration * 2` with `backoffDuration *= 2` (gocritic)
backoffDuration = backoffDuration * 2
^
pkg/controller/admissionchecks/provisioning/controller.go:122:3: commentFormatting: put a space between `//` and comment text (gocritic)
//1.2 workload has no reservation or is finished
^
pkg/controller/admissionchecks/provisioning/controller.go:225:3: commentFormatting: put a space between `//` and comment text (gocritic)
//get the config
^
pkg/cache/snapshot_test.go:991:13: appendAssign: append result not assigned to the same slice (gocritic)
cmpOpts := append(snapCmpOpts,
^
pkg/cache/snapshot_test.go:1498:13: appendAssign: append result not assigned to the same slice (gocritic)
cmpOpts := append(snapCmpOpts,
^
pkg/queue/manager_test.go:237:3: commentFormatting: put a space between `//` and comment text (gocritic)
//nothing
^
pkg/workload/workload.go:397:2: commentFormatting: put a space between `//` and comment text (gocritic)
//reset Evicted condition if present.
^
pkg/controller/jobs/mpijob/mpijob_controller_test.go:294:12: appendAssign: append result not assigned to the same slice (gocritic)
objs := append(tc.priorityClasses, tc.job)
^
pkg/controller/jobs/raycluster/raycluster_controller_test.go:334:12: appendAssign: append result not assigned to the same slice (gocritic)
objs := append(tc.priorityClasses, &tc.job)
^
pkg/scheduler/flavorassigner/flavorassigner.go:450:10: elseif: can replace 'else {if cond {}}' with 'else if cond {}' (gocritic)
} else {
^
pkg/controller/jobs/jobset/jobset_controller_test.go:389:12: appendAssign: append result not assigned to the same slice (gocritic)
objs := append(tc.priorityClasses, tc.job)
^
pkg/visibility/api/rest/pending_workloads_cq_test.go:343:4: ifElseChain: rewrite if-else to switch statement (gocritic)
if tc.wantErrMatch != nil {
^
pkg/visibility/api/rest/pending_workloads_lq_test.go:460:4: ifElseChain: rewrite if-else to switch statement (gocritic)
if tc.wantErrMatch != nil {
^
pkg/controller/admissionchecks/multikueue/admissioncheck.go:98:11: elseif: can replace 'else {if cond {}}' with 'else if cond {}' (gocritic)
} else {
^
pkg/controller/admissionchecks/multikueue/multikueuecluster.go:221:4: commentFormatting: put a space between `//` and comment text (gocritic)
//reconnect if this is the first watch failing.
^
pkg/controller/admissionchecks/multikueue/multikueuecluster.go:241:9: elseif: can replace 'else {if cond {}}' with 'else if cond {}' (gocritic)
} else {
^
pkg/controller/admissionchecks/multikueue/workload_test.go:455:89: commentFormatting: put a space between `//` and comment text (gocritic)
LastTransitionTime: metav1.NewTime(time.Now().Add(-defaulWorkerLostTimeout / 2)), //50% of the timeout
^
pkg/controller/admissionchecks/multikueue/workload_test.go:593:55: commentFormatting: put a space between `//` and comment text (gocritic)
QuotaReservedTime(time.Now().Add(-time.Minute)). //one minute ago
^
cmd/importer/pod/check.go:66:10: elseif: can replace 'else {if cond {}}' with 'else if cond {}' (gocritic)
} else {
^
cmd/importer/pod/import.go:89:3: commentFormatting: put a space between `//` and comment text (gocritic)
//make its admission and update its status
^
pkg/controller/jobs/job/job_webhook.go:101:10: elseif: can replace 'else {if cond {}}' with 'else if cond {}' (gocritic)
} else {
^
pkg/controller/jobs/job/job_controller_test.go:2205:12: appendAssign: append result not assigned to the same slice (gocritic)
objs := append(tc.priorityClasses, &tc.job, utiltesting.MakeResourceFlavor("default").Obj())
^
test/util/util.go:231:30: appendAssign: append result not assigned to the same slice (gocritic)
newWL.Status.Conditions = append(w.Status.Conditions, metav1.Condition{
^
pkg/scheduler/scheduler_test.go:1418:17: appendAssign: append result not assigned to the same slice (gocritic)
allQueues := append(queues, tc.additionalLocalQueues...)
^
pkg/scheduler/scheduler_test.go:1419:24: appendAssign: append result not assigned to the same slice (gocritic)
allClusterQueues := append(clusterQueues, tc.additionalClusterQueues...)
^
pkg/scheduler/scheduler_test.go:1500:6: ifElseChain: rewrite if-else to switch statement (gocritic)
if !workload.HasQuotaReservation(w.Obj) {
^
pkg/scheduler/scheduler_test.go:2089:6: ifElseChain: rewrite if-else to switch statement (gocritic)
if !workload.IsAdmitted(w.Obj) {
^
pkg/controller/jobs/pod/expectations.go:44:83: captLocal: `UIDs' should not be capitalized (gocritic)
func (e *expectationsStore) ExpectUIDs(log logr.Logger, key types.NamespacedName, UIDs []types.UID) {
^
test/integration/controller/jobs/jobset/jobset_controller_test.go:161:3: commentFormatting: put a space between `//` and comment text (gocritic)
//gomega.Expect(k8sClient.Get(ctx, wlLookupKey, createdWorkload)).Should(gomega.Succeed())
^
cmd/kueue/main.go:286:3: exitAfterDefer: os.Exit will exit, and `defer setupLog.Info("Probe endpoints are configured on healthz and readyz")` will not run (gocritic)
os.Exit(1)
^
make: *** [Makefile:217: ci-lint] Error 1
It's because gocritic behavior in golangci-lint v1.57.2 slightly changes (see golangci/golangci-lint#4335)
There was a problem hiding this comment.
should we use disable-all instead?
There was a problem hiding this comment.
We can't use disable-all as it will disable all checks, including the defaults.
You can view the enabled gocritic checks by setting the environment variable GL_DEBUG=gocritic:
❯ GL_DEBUG=gocritic golangci-lint run
...
DEBU [gocritic] Enabled by config checks (1): [dupImport]
DEBU [gocritic] Disabled by config checks (8): [appendAssign assignOp captLocal commentFormatting deprecatedComment elseif exitAfterDefer ifElseChain]
DEBU [gocritic] Final used checks (27): [argOrder badCall badCond caseOrder codegenComment defaultCaseOrder dupArg dupBranchBody dupCase dupImport dupSubExpr flagDeref flagName mapKey newDeref offBy1 regexpMust singleCaseSwitch sloppyLen sloppyTypeAssert switchTrue typeSwitchVar underef unlambda unslice valSwap wrapperFunc]
| @@ -21,10 +24,21 @@ linters-settings: | |||
| # Settings for enabling and disabling linters | |||
| linters: | |||
| enable: | |||
| - copyloopvar | |||
There was a problem hiding this comment.
I found that this is implemented by my co-worker :)
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
| @@ -1,17 +1,20 @@ | |||
| # golangci-lint configuration file | |||
| # see: https://golangci-lint.run/usage/configuration/ | |||
|
|
|||
| # Options for analysis running | |||
| run: | |||
| # Which dirs to skip: they won't be analyzed; | |||
There was a problem hiding this comment.
why remove this?
IIUC, issues.exclude-dirs would skip the reporting for this folder, but it would still analyze it. Or not?
There was a problem hiding this comment.
run.skip-dirs has been deprecated since golangci-lint v1.57.0 and replaced by issues.exclude-dirs. These options are technically identical. For a detailed explanation, see golangci/golangci-lint#4509.
Unfortunately, golangci-lint does not provide a way to skip directory analysis, run.skip-dirs lies. See golangci/golangci-lint#1832
| disabled-checks: # temporarily disabled checks, will fix them later | ||
| - appendAssign | ||
| - assignOp | ||
| - captLocal | ||
| - commentFormatting | ||
| - deprecatedComment | ||
| - elseif | ||
| - exitAfterDefer | ||
| - ifElseChain |
There was a problem hiding this comment.
should we use disable-all instead?
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: a6e952bfc18aa52b24ed0f55b3661445df0147e3
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor, alexandear 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 |
k8s-ci-robot
added
the
approved
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Clean up unnecessary loop variable copying and enable the
copyloopvarlinter for detecting this redundant variable copying.Which issue(s) this PR fixes:
Special notes for your reviewer:
After the project upgraded to Go version 1.22 in #1895, copying variables inside a
forloop became unnecessary. See this blog post for a detailed explanation.The
copyloopvarlinter is only available fromgolangci-lintv1.57 onwards, so we also need to update this tool.Does this PR introduce a user-facing change?