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.

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

Update .golangci.yml config to resolve deprecation warnings #4082

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
31 changes: 19 additions & 12 deletions .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,6 @@ run:
# list of build tags, all linters use it. Default is empty list.
build-tags:

# which dirs to skip: they won't be analyzed;
# can use regexp here: generated.*, regexp is applied on full path;
# default value is empty list, but next dirs are always skipped independently
# from this option's value:
# vendor$, third_party$, testdata$, examples$, Godeps$, builtin$
skip-dirs:
- test/sdk/restapi

# which files to skip: they will be analyzed, but issues from them
# won't be reported. Default value is empty list, but there is
# no need to include all autogenerated files, we confidently recognize
Expand All @@ -34,8 +26,15 @@ run:
modules-download-mode: vendor
# output configuration options
output:
# colored-line-number|line-number|json|tab|checkstyle, default is "colored-line-number"
format: colored-line-number
# the formats used to render issues
# formats: colored-line-number, line-number, json, colored-tab,
# tab, html, checkstyle, code-climate, junit-xml,
# junit-xml-extended, github-actions, teamcity, sarif
# output path can be either `stdout`, `stderr`
# or path to the file to write to
formats:
- format: colored-line-number
path: stdout

# print lines of code with issue, default is true
print-issued-lines: true
Expand All @@ -45,20 +44,21 @@ output:
linters:
enable:
- bodyclose
- copyloopvar
- dupl
- exportloopref
Copy link
Collaborator

Choose a reason for hiding this comment

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

The warning for this was:

WARN The linter 'exportloopref' is deprecated (since v1.60.2) due to: Since Go1.22 (loopvar) this linter is no longer relevant. Replaced by copyloopvar.

Should we replace it, rather than just delete it? (Same for all the other ones as well?)

Copy link
Contributor Author

@paulinek13 paulinek13 Jan 6, 2025

Choose a reason for hiding this comment

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

Thank you for pointing that out! I’ve updated the changes to replace the deprecated linters rather than just removing them.

Copy link
Collaborator

Choose a reason for hiding this comment

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

oooh! My bad! I read it as addition, not subtraction.

Sorry! Doing too many things at once 🤦🏻

- goconst
- gocritic
- gocyclo
- goimports
- gosimple
- govet
- megacheck
- misspell
- nakedret
- revive
- staticcheck
- unconvert
- unparam
- unused
linters-settings:
govet:
disable-all: false
Expand Down Expand Up @@ -88,3 +88,10 @@ issues:
- path: (.+)_test\.go|^test/|^cmd/.*|^pkg/apis/.*
# fieldalignment is in the `govet` linter, so exclude based on text instead of all of govet
text: '^fieldalignment: .*'

# which dirs to exclude: issues from them won't be reported
# default dirs are skipped independently of this option's value
# (see exclude-dirs-use-default)
# default: []
exclude-dirs:
- test/sdk/restapi
1 change: 0 additions & 1 deletion pkg/allocation/converters/converter.go
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,6 @@ func convertInternalLabelSelectorToLabelSelector(in *metav1.LabelSelector) *pb.L
func convertInternalLabelSelectorsToLabelSelectors(in []allocationv1.GameServerSelector) []*pb.GameServerSelector {
var result []*pb.GameServerSelector
for _, l := range in {
l := l
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you explain this pattern? It seems.... weird? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After enabling the copyloopvar linter, I noticed it flagged instances of redundant code, specifically unnecessary copies of loop variables. For example:

pkg/allocation/converters/converter.go:289:3: The copy of the 'for' variable "l" can be deleted (Go 1.22+) (copyloopvar)
                l := l

I’ve cleaned up these instances to resolve the linter errors.
Context: https://go.dev/blog/loopvar-preview

Copy link
Contributor Author

@paulinek13 paulinek13 Jan 6, 2025

Choose a reason for hiding this comment

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

Sorry, I was sure I've already added this comment
I hope it answers your question :)

c := convertInternalGameServerSelectorToGameServer(&l)
result = append(result, c)
}
Expand Down
4 changes: 0 additions & 4 deletions pkg/allocation/converters/converter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -626,7 +626,6 @@ func TestConvertAllocationRequestToGameServerAllocation(t *testing.T) {
},
}
for _, tc := range tests {
tc := tc
t.Run(tc.name, func(t *testing.T) {
t.Parallel()

Expand Down Expand Up @@ -834,7 +833,6 @@ func TestConvertGSAToAllocationRequest(t *testing.T) {
},
}
for _, tc := range tests {
tc := tc
t.Run(tc.name, func(t *testing.T) {
t.Parallel()

Expand Down Expand Up @@ -1370,7 +1368,6 @@ func TestConvertGSAToAllocationResponse(t *testing.T) {
},
}
for _, tc := range tests {
tc := tc
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
runtime.FeatureTestMutex.Lock()
Expand Down Expand Up @@ -1561,7 +1558,6 @@ func TestConvertAllocationResponseToGSA(t *testing.T) {
},
}
for _, tc := range tests {
tc := tc
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
runtime.FeatureTestMutex.Lock()
Expand Down
2 changes: 0 additions & 2 deletions pkg/gameserverallocations/allocation_cache_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -174,8 +174,6 @@ func TestAllocationCacheListSortedGameServers(t *testing.T) {
}

for k, v := range fixtures {
k := k
v := v
t.Run(k, func(t *testing.T) {
// deliberately not resetting the Feature state, to catch any possible unknown regressions with the
// new feature flags
Expand Down
2 changes: 1 addition & 1 deletion pkg/sdkserver/localsdk.go
Original file line number Diff line number Diff line change
Expand Up @@ -882,7 +882,7 @@ func (l *LocalSDKServer) setGameServerFromFilePath(filePath string) error {
l.logger.WithField("filePath", filePath).Info("Reading GameServer configuration")

reader, err := os.Open(filePath) // nolint: gosec
defer reader.Close() // nolint: megacheck,errcheck
defer reader.Close() // nolint: staticcheck,errcheck

if err != nil {
return err
Expand Down
6 changes: 0 additions & 6 deletions pkg/sdkserver/localsdk_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -160,9 +160,6 @@ func TestLocalSDKServerSetLabel(t *testing.T) {
}

for k, v := range fixtures {
// pin variables here, see scopelint for details
k := k
v := v
t.Run(k, func(t *testing.T) {
ctx := context.Background()
e := &sdk.Empty{}
Expand Down Expand Up @@ -470,9 +467,6 @@ func TestLocalSDKServerPlayerConnectAndDisconnect(t *testing.T) {
}

for k, v := range fixtures {
// pin variables here, see https://github.com/kyoh86/scopelint for the details
k := k
v := v
t.Run(k, func(t *testing.T) {
var l *LocalSDKServer
var err error
Expand Down
9 changes: 0 additions & 9 deletions test/e2e/fleet_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,6 @@ func TestFleetScaleUpEditAndScaleDown(t *testing.T) {
fixtures := []bool{true, false}

for _, usePatch := range fixtures {
usePatch := usePatch
t.Run("Use fleet Patch "+fmt.Sprint(usePatch), func(t *testing.T) {
t.Parallel()
ctx := context.Background()
Expand Down Expand Up @@ -633,7 +632,6 @@ func TestScaleFleetUpAndDownWithGameServerAllocation(t *testing.T) {
fixtures := []bool{false, true}

for _, usePatch := range fixtures {
usePatch := usePatch
t.Run("Use fleet Patch "+fmt.Sprint(usePatch), func(t *testing.T) {
t.Parallel()

Expand Down Expand Up @@ -714,8 +712,6 @@ func TestFleetUpdates(t *testing.T) {
}

for k, v := range fixtures {
k := k
v := v
t.Run(k, func(t *testing.T) {
t.Parallel()
client := framework.AgonesClient.AgonesV1()
Expand Down Expand Up @@ -1380,7 +1376,6 @@ func TestFleetRecreateGameServers(t *testing.T) {
podClient := framework.KubeClient.CoreV1().Pods(framework.Namespace)

for _, gs := range list.Items {
gs := gs
pod, err := podClient.Get(ctx, gs.ObjectMeta.Name, metav1.GetOptions{})
assert.NoError(t, err)

Expand All @@ -1392,7 +1387,6 @@ func TestFleetRecreateGameServers(t *testing.T) {
}},
"gameserver shutdown": {f: func(t *testing.T, list *agonesv1.GameServerList) {
for _, gs := range list.Items {
gs := gs
var reply string
reply, err := framework.SendGameServerUDP(t, &gs, "EXIT")
if err != nil {
Expand All @@ -1410,7 +1404,6 @@ func TestFleetRecreateGameServers(t *testing.T) {
}},
"gameserver unhealthy": {f: func(t *testing.T, list *agonesv1.GameServerList) {
for _, gs := range list.Items {
gs := gs
var reply string
reply, err := framework.SendGameServerUDP(t, &gs, "UNHEALTHY")
if err != nil {
Expand All @@ -1423,8 +1416,6 @@ func TestFleetRecreateGameServers(t *testing.T) {
}

for k, v := range tests {
k := k
v := v
t.Run(k, func(t *testing.T) {
t.Parallel()
client := framework.AgonesClient.AgonesV1()
Expand Down
4 changes: 0 additions & 4 deletions test/e2e/gameserverallocation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ func TestCreateFleetAndGameServerAllocate(t *testing.T) {
fixtures := []apis.SchedulingStrategy{apis.Packed, apis.Distributed}

for _, strategy := range fixtures {
strategy := strategy
t.Run(string(strategy), func(t *testing.T) {
t.Parallel()
ctx := context.Background()
Expand Down Expand Up @@ -999,7 +998,6 @@ func TestMultiClusterAllocationOnLocalCluster(t *testing.T) {

fixtures := []apis.SchedulingStrategy{apis.Packed, apis.Distributed}
for _, strategy := range fixtures {
strategy := strategy
t.Run(string(strategy), func(t *testing.T) {
if strategy == apis.Distributed {
framework.SkipOnCloudProduct(t, "gke-autopilot", "Autopilot does not support Distributed scheduling")
Expand Down Expand Up @@ -1138,8 +1136,6 @@ func TestCreateFullFleetAndCantGameServerAllocate(t *testing.T) {
fixtures := []apis.SchedulingStrategy{apis.Packed, apis.Distributed}

for _, strategy := range fixtures {
strategy := strategy

t.Run(string(strategy), func(t *testing.T) {
if strategy == apis.Distributed {
framework.SkipOnCloudProduct(t, "gke-autopilot", "Autopilot does not support Distributed scheduling")
Expand Down
Loading