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

Conversation

paulinek13
Copy link
Contributor

What type of PR is this?

/kind cleanup

What this PR does / Why we need it:

This PR updates the .golangci.yml configuration file to resolve deprecation warnings.
The changes include:

  • Replacing the deprecated run.skip-dirs with issues.exclude-dirs.
  • Updating output.format to output.formats.
  • Removing the deprecated megacheck and exportloopref linters.
  • Updating code comments to reflect the changes.

These updates are necessary to keep the linting process up-to-date and free from deprecation warnings.

Which issue(s) this PR fixes:

Closes #4081

Special notes for your reviewer:

  • Ran make lint to verify that the warnings have been resolved.

Copy link

google-cla bot commented Jan 6, 2025

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@markmandel
Copy link
Collaborator

/gcbrun

@agones-bot
Copy link
Collaborator

Build Failed 😭

Build Id: 9782d29a-dc60-41bf-99b1-56df5acad91a

Status: FAILURE

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@markmandel
Copy link
Collaborator

Looks like a flake in e2e:

VERBOSE:     framework.go:346: 
VERBOSE:         	Error Trace:	/go/src/agones.dev/agones/test/e2e/framework/framework.go:346
VERBOSE:         	            				/go/src/agones.dev/agones/test/e2e/fleetautoscaler_test.go:1488
VERBOSE:         	Error:      	Received unexpected error:
VERBOSE:         	            	context deadline exceeded
VERBOSE:         	Test:       	TestListAutoscalerAllocated/Scale_Up_Buffer_Percent
VERBOSE:         	Messages:   	error waiting for fleet condition on fleet: simple-fleet-1.09s85n
VERBOSE: --- FAIL: TestListAutoscalerAllocated/Scale_Up_Buffer_Percent (311.66s)
VERBOSE: --- FAIL: TestListAutoscalerAllocated (311.66s)

@@ -46,13 +45,11 @@ linters:
enable:
- bodyclose
- 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 🤦🏻

@markmandel
Copy link
Collaborator

/gcbrun

@agones-bot
Copy link
Collaborator

Build Succeeded 🥳

Build Id: 6022e24d-fce6-4418-ab67-79d85c49f678

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/4082/head:pr_4082 && git checkout pr_4082
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.47.0-dev-ec717e3

…on warnings

- Replace deprecated `run.skip-dirs` with `issues.exclude-dirs`
- Update `output.format` to `output.formats`
- Replace deprecated linters:
  - `megacheck` (split into `gosimple`, `staticcheck`, and `unused`)
  - `exportloopref` (no longer relevant since Go 1.22, replaced by `copyloopvar`)
- Remove redundant variable copies flagged by `copyloopvar` (context: https://go.dev/blog/loopvar-preview)
- Adjust `defer` statement comments in `localsdk.go` to use `staticcheck` instead of the deprecated `megacheck`

Signed-off-by: Paulina Kalicka <[email protected]>
@paulinek13 paulinek13 force-pushed the fix/lint-deprecation-warnings branch from ec717e3 to 6e41528 Compare January 6, 2025 19:35
@markmandel
Copy link
Collaborator

/gcbrun

@@ -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 :)

@agones-bot
Copy link
Collaborator

Build Succeeded 🥳

Build Id: dd857123-5cca-4d29-9e58-730ff8dd7a2a

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/4082/head:pr_4082 && git checkout pr_4082
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.47.0-dev-6e41528

Copy link
Collaborator

@markmandel markmandel left a comment

Choose a reason for hiding this comment

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

Nice!

@@ -46,13 +45,11 @@ linters:
enable:
- bodyclose
- 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.

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

Sorry! Doing too many things at once 🤦🏻

@markmandel markmandel merged commit 0366002 into googleforgames:main Jan 6, 2025
4 checks passed
@markmandel markmandel added kind/cleanup Refactoring code, fixing up documentation, etc area/build-tools Development tooling. I.e. pretty much everything in the `build` directory. labels Jan 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build-tools Development tooling. I.e. pretty much everything in the `build` directory. kind/cleanup Refactoring code, fixing up documentation, etc kind/other size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make lint deprecations
3 participants