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-lint in Makefile #17647

Merged
merged 8 commits into from
Nov 17, 2021
Merged

Conversation

Gusted
Copy link
Contributor

@Gusted Gusted commented Nov 15, 2021

  • Partially resolvess Tracking issue: Updating golangci-lint #17596
  • Download specific version(v1.43.0) by default.
  • If current installed version is older than the minium version, it will download the mininium required version.
  • Update the install script to avoid deprecated error golangci/golangci-lint err this script is deprecated, please do not use it anymore. check https://github.com/goreleaser/godownloader/issues/207

- Partially resolvess go-gitea#17596
- Download specific version(v1.43.0) by default.
- If current installed version is older than the minium version, it will
download the mininium required version.
- Update the install script to avoid deprecated error
`golangci/golangci-lint err this script is deprecated, please do not use
it anymore. check https://github.com/goreleaser/godownloader/issues/207`
@silverwind
Copy link
Member

silverwind commented Nov 15, 2021

I think we should do away with globally installed modules as different projects can require different versions of the linter, resulting in conflicts. Does golang offer a solution to install such devDependencies locally yet (Without fake imports)?

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 15, 2021
@Gusted
Copy link
Contributor Author

Gusted commented Nov 15, 2021

Does golang offer a solution to install such devDependencies locally yet (Without fake imports)?

Does golang offer such solution officially? No, it doesn't.
Does golang support such solution possibly? Yeah-ish, see golang/go#25922 (comment) it's the best thing you can get, but it feels like a hack for me.

@silverwind
Copy link
Member

Yeah, such fake imports are unacceptable I think because it will download/build these dependencies even on regular builds.

@Gusted
Copy link
Contributor Author

Gusted commented Nov 15, 2021

Yeah, such fake imports are unacceptable I think because it will download/build these dependencies even on regular builds.

Not if you have a custom XXX.mod file like mentioned, it will only download the go.mod's dependencies by default.

@silverwind
Copy link
Member

For the version check, why not do it like it's done for go/node? It's much simpler and should suffice:

gitea/Makefile

Lines 200 to 205 in ab13797

go-check:
$(eval GO_VERSION := $(shell printf "%03d%03d%03d" $(shell $(GO) version | grep -Eo '[0-9]+\.[0-9.]+' | tr '.' ' ');))
@if [ "$(GO_VERSION)" -lt "$(MIN_GO_VERSION)" ]; then \
echo "Gitea requires Go 1.16 or greater to build. You can get it at https://golang.org/dl/"; \
exit 1; \
fi

@Gusted
Copy link
Contributor Author

Gusted commented Nov 15, 2021

For the version check, why not do it like it's done for go/node? It's much simpler and should suffice:

gitea/Makefile

Lines 200 to 205 in ab13797

go-check:
$(eval GO_VERSION := $(shell printf "%03d%03d%03d" $(shell $(GO) version | grep -Eo '[0-9]+\.[0-9.]+' | tr '.' ' ');))
@if [ "$(GO_VERSION)" -lt "$(MIN_GO_VERSION)" ]; then \
echo "Gitea requires Go 1.16 or greater to build. You can get it at https://golang.org/dl/"; \
exit 1; \
fi

Not sure how I missed that part, the version check is now simplified, thanks.

@Gusted
Copy link
Contributor Author

Gusted commented Nov 15, 2021

Yeah, such fake imports are unacceptable I think because it will download/build these dependencies even on regular builds.

Not if you have a custom XXX.mod file like mentioned, it will only download the go.mod's dependencies by default.

NVM that, it still requires to have a file with fake imports.

@techknowlogick
Copy link
Member

This will also need to be updated in the following repo: https://gitea.com/gitea/test-env/

@silverwind
Copy link
Member

Download does not work, it tries on non-existing URL https://raw.githubusercontent.com/golangci/golangci-lint/001043000.

@Gusted
Copy link
Contributor Author

Gusted commented Nov 15, 2021

This will also need to be updated in the following repo: https://gitea.com/gitea/test-env/

https://gitea.com/gitea/test-env/pulls/10

@lunny
Copy link
Member

lunny commented Nov 16, 2021

Makefile Outdated Show resolved Hide resolved
Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

@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 Nov 16, 2021
@silverwind
Copy link
Member

silverwind commented Nov 16, 2021

It bothers me a bit that the checks work differently the go/node which are defined with zero-padded numbers. Maybe make those checks match by either formatting/de-formatting their versions consistently?

@Gusted
Copy link
Contributor Author

Gusted commented Nov 16, 2021

Voila - done.

@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 Nov 16, 2021
@codecov-commenter
Copy link

Codecov Report

Merging #17647 (ba6eee6) into main (8fdc524) will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #17647      +/-   ##
==========================================
- Coverage   45.35%   45.34%   -0.02%     
==========================================
  Files         799      799              
  Lines       89062    89062              
==========================================
- Hits        40393    40382      -11     
- Misses      42163    42174      +11     
  Partials     6506     6506              
Impacted Files Coverage Δ
models/gpg_key_common.go 59.67% <0.00%> (-4.84%) ⬇️
modules/queue/workerpool.go 48.47% <0.00%> (-3.44%) ⬇️
modules/queue/queue_disk_channel.go 60.94% <0.00%> (-1.78%) ⬇️
services/pull/pull.go 42.19% <0.00%> (+0.40%) ⬆️
modules/log/file.go 75.20% <0.00%> (+1.60%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 42670e6...ba6eee6. Read the comment docs.

@lunny lunny merged commit 21f6c0b into go-gitea:main Nov 17, 2021
@Gusted Gusted deleted the update-golangci-lint branch November 24, 2021 08:26
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
* Update golangci-lint in Makefile

- Partially resolvess go-gitea#17596
- Download specific version(v1.43.0) by default.
- If current installed version is older than the minium version, it will
download the mininium required version.
- Update the install script to avoid deprecated error
`golangci/golangci-lint err this script is deprecated, please do not use
it anymore. check https://github.com/goreleaser/godownloader/issues/207`

* Simplify golangci-lint version check

* Fix version conversion

* Add version that's downloading

Co-authored-by: zeripath <[email protected]>

* Consistency

Co-authored-by: zeripath <[email protected]>
Co-authored-by: Lunny Xiao <[email protected]>
Co-authored-by: techknowlogick <[email protected]>
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/code-linting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants