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.

Sign up for GitHub

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

Jump to bottom

Remove unneeded loop variable copying #1946

Merged
merged 1 commit into from
Apr 11, 2024
Merged

Remove unneeded loop variable copying #1946

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 20 additions & 6 deletions .golangci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

why remove this?

IIUC, issues.exclude-dirs would skip the reporting for this folder, but it would still analyze it. Or not?

Copy link
Contributor Author

@alexandear alexandear Apr 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

skip-dirs:
- bin

# Settings of specific linters
linters-settings:
gocritic:
enabled-checks:
- dupImport
disabled-checks: # temporarily disabled checks, will fix them later
- appendAssign
- assignOp
- captLocal
- commentFormatting
- deprecatedComment
- elseif
- exitAfterDefer
- ifElseChain
Comment on lines +9 to +17
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need these?

Copy link
Contributor Author

@alexandear alexandear Apr 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

should we use disable-all instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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]

goimports:
local-prefixes: sigs.k8s.io/kueue
govet:
Expand All @@ -21,10 +24,21 @@ linters-settings:
# Settings for enabling and disabling linters
linters:
enable:
- copyloopvar
Copy link
Member

Choose a reason for hiding this comment

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

I found that this is implemented by my co-worker :)

- dupword
- ginkgolinter
- gocritic
- goimports
- govet
- misspell
- unconvert

# Settings related to issues
issues:
# Which dirs to exclude: issues from them won't be reported
exclude-dirs:
- bin
# Show all issues from a linter
max-issues-per-linter: 0
# Show all issues with the same text
max-same-issues: 0
2 changes: 1 addition & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ PROJECT_DIR := $(shell dirname $(abspath $(lastword $(MAKEFILE_LIST))))
GOLANGCI_LINT = $(PROJECT_DIR)/bin/golangci-lint
.PHONY: golangci-lint
golangci-lint: ## Download golangci-lint locally if necessary.
@GOBIN=$(PROJECT_DIR)/bin GO111MODULE=on $(GO_CMD) install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.56.2
@GOBIN=$(PROJECT_DIR)/bin GO111MODULE=on $(GO_CMD) install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.57.2

CONTROLLER_GEN = $(PROJECT_DIR)/bin/controller-gen
.PHONY: controller-gen
Expand Down
1 change: 0 additions & 1 deletion pkg/queue/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,6 @@ func (m *Manager) AddLocalQueue(ctx context.Context, q *kueue.LocalQueue) error
return fmt.Errorf("listing workloads that match the queue: %w", err)
}
for _, w := range workloads.Items {
w := w
if workload.HasQuotaReservation(&w) {
continue
}
Expand Down
1 change: 0 additions & 1 deletion pkg/queue/manager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,7 +460,6 @@ func TestStatus(t *testing.T) {
}
}
for _, wl := range workloads {
wl := wl
manager.AddOrUpdateWorkload(&wl)
}

Expand Down
4 changes: 1 addition & 3 deletions pkg/util/priority/priority.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,7 @@ func getDefaultPriorityClass(ctx context.Context, client client.Client) (*schedu
// In case more than one global default priority class is added as a result of a race condition,
// we pick the one with the lowest priority value.
var defaultPC *schedulingv1.PriorityClass
for _, pci := range pcs.Items {
item := pci

for _, item := range pcs.Items {
if item.GlobalDefault {
if defaultPC == nil || defaultPC.Value > item.Value {
defaultPC = &item
Expand Down
2 changes: 0 additions & 2 deletions pkg/util/priority/priority_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,6 @@ func TestGetPriorityFromPriorityClass(t *testing.T) {
}

for desc, tt := range tests {
tt := tt
t.Run(desc, func(t *testing.T) {
t.Parallel()

Expand Down Expand Up @@ -200,7 +199,6 @@ func TestGetPriorityFromWorkloadPriorityClass(t *testing.T) {
}

for desc, tt := range tests {
tt := tt
t.Run(desc, func(t *testing.T) {
t.Parallel()

Expand Down