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

Use Go 1.21 for golangci-lint #26786

Merged
merged 2 commits into from
Aug 29, 2023
Merged

Use Go 1.21 for golangci-lint #26786

merged 2 commits into from
Aug 29, 2023

Conversation

harryzcy
Copy link
Contributor

No description provided.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 29, 2023
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 29, 2023
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Aug 29, 2023
@puni9869
Copy link
Member

puni9869 commented Aug 29, 2023 •
edited
Loading

Please update at other places also. Means whole CI should run on 1.21 version other wise we will face failures.
image

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Aug 29, 2023
@puni9869 puni9869 added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Aug 29, 2023
@silverwind
Copy link
Member

Please update at other places also. Means whole CI should run on 1.21 version other wise we will face failures. image

At least gitea/test_env does not have a 1.21 tag yet:

https://hub.docker.com/r/gitea/test_env/tags
https://gitea.com/gitea/test-env

@silverwind
Copy link
Member

As the future of test_env is uncertain (we don't need it any more with actions), I'l fine to merge this as-is now.

@silverwind silverwind merged commit 3507cad into go-gitea:main Aug 29, 2023
23 checks passed
@GiteaBot GiteaBot added this to the 1.21.0 milestone Aug 29, 2023
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Aug 29, 2023
@harryzcy
Copy link
Contributor Author

Somehow the tests didn't caught a deprecation error, but running golangci-lint locally gives me:

models/migrations/v1_16/v210.go:132:23: SA1019: elliptic.Marshal has been deprecated since Go 1.21: for ECDH, use the crypto/ecdh package. This function returns an encoding equivalent to that of PublicKey.Bytes in crypto/ecdh. (staticcheck)
                                        PublicKey:       elliptic.Marshal(elliptic.P256(), parsed.PubKey.X, parsed.PubKey.Y),

The crypto/ecdh package was added in Go 1.20, and the old API is deprecated in Go 1.21.
But also Gitea v1.16 was released long ago. Do we still want to make this change?

@silverwind
Copy link
Member

silverwind commented Aug 29, 2023 •
edited
Loading

That would require us to bump minimum go version to 1.21 it seems. I think we can do this on main branch sometimes next week once the release/v1.21 branch is cut.

Also, wonder why the deprecation did not show on CI. Ideally we should probably update go.mod and .golangci.yml only in-sync.

@silverwind silverwind mentioned this pull request Aug 29, 2023
Merged
@harryzcy
Copy link
Contributor Author

As of 1.21, the version in go.mod becomes the strict minimum requirement, so it will always lag everywhere else by 1 minor version.

To improve forwards compatibility, Go 1.21 now reads the go line in a go.work or go.mod file as a strict minimum requirement: go 1.21.0 means that the workspace or module cannot be used with Go 1.20 or with Go 1.21rc1.

@silverwind
Copy link
Member

silverwind commented Aug 29, 2023 •
edited
Loading

Found the bug in our actions setup, which is why this did not run the linter when only .golangci.yml changed:

#26799

Afraid we might have to revert this because now we should have a lint failure on main branch because of it.

@harryzcy
Copy link
Contributor Author

I think fixing it now may still be fine, since crypto/ecdh is added in 1.20. As long as we don't need to support 1.19, the code should also compile.

@silverwind
Copy link
Member

Ah, yes in that case we can keep 1.20 minimum. I thought it was only available with 1.21.

@harryzcy harryzcy deleted the go-1.21-lint branch August 29, 2023 18:11
@silverwind
Copy link
Member

@harryzcy could you raise a PR that fixes the deprecation on main branch?

@harryzcy
Copy link
Contributor Author

@harryzcy could you raise a PR that fixes the deprecation on main branch?

Done in #26800

@harryzcy harryzcy mentioned this pull request Aug 29, 2023
Merged
silverwind pushed a commit that referenced this pull request Aug 29, 2023
@harryzcy
Replace deprecated elliptic.Marshal (#26800)
2d9249b 
In PR #26786, the Go version for golangci-lint is bumped to 1.21. This
causes the following error:

```
models/migrations/v1_16/v210.go:132:23: SA1019: elliptic.Marshal has been deprecated since Go 1.21: for ECDH, use the crypto/ecdh package. This function returns an encoding equivalent to that of PublicKey.Bytes in crypto/ecdh. (staticcheck)
                                        PublicKey:       elliptic.Marshal(elliptic.P256(), parsed.PubKey.X, parsed.PubKey.Y),
```

The change now uses [func (*PublicKey)
ECDH](https://pkg.go.dev/crypto/ecdsa#PublicKey.ECDH), which is added in
Go 1.20.
silverwind added a commit that referenced this pull request Aug 30, 2023
@silverwind
Add various missing files-changed dependencies (#26799)
7bc80cb 
We were missing a number of config files like `.golangci.yml` in the
dependencies for the pull request pipelines, which resulted in the
linting not running for #26786
because only `.golangci.yml` had changed.
zjjhot added a commit to zjjhot/gitea that referenced this pull request Aug 30, 2023
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
91a5317 
* giteaofficial/main:
  Use `Set[Type]` instead of `map[Type]bool/struct{}`. (go-gitea#26804)
  Fix verifyCommits error when push a new branch (go-gitea#26664)
  Fix Uint8Array comparisons and update vitest (go-gitea#26805)
  Add various missing files-changed dependencies (go-gitea#26799)
  Improve flex list item padding (go-gitea#26779)
  Include the GITHUB_TOKEN/GITEA_TOKEN secret for fork pull requests (go-gitea#26759)
  feat(API): add route and implementation for creating/updating repository secret (go-gitea#26766)
  Replace deprecated `elliptic.Marshal` (go-gitea#26800)
  Updating the js libraries to latest version. (go-gitea#26795)
  Fix some slice append usages (go-gitea#26778)
  Use Go 1.21 for golangci-lint (go-gitea#26786)
  Fix notification circle (border-radius) (go-gitea#26794)
  Fix context filter has no effect in dashboard (go-gitea#26695)
  Add default label in branch select list (go-gitea#26697)
  Remove redundant nil check in `WalkGitLog` (go-gitea#26773)
  Remove fomantic `item` module (go-gitea#26775)
  Update info regarding internet connection for build (go-gitea#26776)
  Fix being unable to use a repo that prohibits accepting PRs as a PR source. (go-gitea#26785)
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Nov 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@delvh delvh delvh approved these changes

@lunny lunny lunny approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
1.21.0
Development

Successfully merging this pull request may close these issues.
7 participants
@harryzcy @puni9869 @silverwind @lunny @delvh @GiteaBot @denyskon