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 to Go 1.18 #72

Merged
merged 7 commits into from Jul 9, 2022
Merged

Update to Go 1.18 #72

merged 7 commits into from Jul 9, 2022

Conversation

pedreviljoen
Copy link
Contributor

As per below issue:

#68

This PR updates Go to version 1.18

@pedreviljoen pedreviljoen requested a review from a team as a code owner April 12, 2022 19:38
@coveralls
Copy link

coveralls commented Apr 12, 2022

Pull Request Test Coverage Report for Build 2413133311

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 77.309%

Totals Coverage Status
Change from base Build 2309967076: 0.0%
Covered Lines: 385
Relevant Lines: 498

💛 - Coveralls

@andrerfcsantos
Copy link
Member

andrerfcsantos commented Apr 12, 2022

@pedreviljoen Thanks for this.

This PR can't be merged just yet. The reason is that we are using golangci/golangci-lint-action@v2 to run golangci-lint at v1.43 with the default checks:

- name: Run linters
uses: golangci/golangci-lint-action@v2
with:
version: v1.43

However, golangci-lint in v1.43 doesn't understand generics. The support was only added in v1.45. Bumping the version of golangci-lint will probably make CI pass.

However, there's another nuance to this. At version 1.45.2, golangci-lint uses the version v0.2.2 of honnef.co/go/tools which provides the staticcheck check:

https://github.com/golangci/golangci-lint/blob/8bdc4d3f8044b1a20e10a9f519b5f738e8188877/go.mod#L99

This check is used in our Go repositories, and it is enabled by default by golangci-lint. But support for generics was only added in v0.3.0 of staticcheck. Checking the repository of golangci-lint, I see they already updated to v0.3.0 of staticcheck on master:

https://github.com/golangci/golangci-lint/blob/ff93f2715709ab084d0004b197f5ba84116a65ee/go.mod#L101

However, this is only on master, it's not on any of the official releases. It is expected to be included on the next release of golangci-lint. I would wait for the next release golangci-lint to contain v0.3.0 of staticcheck and only update then.

This should also be merged together with the update to the analyzer.

.gitignore Outdated Show resolved Hide resolved
@pedreviljoen
Copy link
Contributor Author

@andrerfcsantos
Ok cool, let's wait for those peer dependencies to be more stable. Is there a way we can keep an eye on those?

@andrerfcsantos
Copy link
Member

The best way is to keep track of new releases of golangci/golangci-lint

There's also this issue keeping track of the support for 1.18 of all the linters of golangci-lint: golangci/golangci-lint#2649

In our case, I think it's fine to wait just for the next release of golangci-lint to update staticcheck to support go 1.18, the other are not as important. Also, like that issue describes, this problem with lack of support only happens when the go.mod for the module being checked declares 1.18 - we don't have any currently in the main repo for the Go track and I won't think we'll have one for some time. Even when we do, I think having just staticcheck (and some other basic checks) is enough and we shouldn't block students to use Go 1.18 just because we are waiting for perfect support for go 1.18 in the tools for our CI (which doesn't affect students directly). We can always do a second upgrade later when those tools add more support for go 1.18.

@andrerfcsantos
Copy link
Member

@pedreviljoen golangci-lint released an update today (1.46.0) that updates staticcheck to work with Go 1.18: https://github.com/golangci/golangci-lint/releases/tag/v1.46.0

The relevant changelog lines for us are:

golangci/golangci-lint@e4945f7 build(deps): bump honnef.co/go/tools from 0.2.2 to 0.3.0 (golangci/golangci-lint#2738)
golangci/golangci-lint@5c77c1b build(deps): bump honnef.co/go/tools from 0.3.0 to 0.3.1 (golangci/golangci-lint#2780)
[...]
golangci/golangci-lint@f5b92e1 staticcheck: re-enable for go1.18 (golangci/golangci-lint#2746)

So it would be great if you could update this PR to use golangci-lint v1.46.0. It's probably also a good idea to update the versions for our actions:

  • actions/setup-go@v2 -> actions/setup-go@v3
  • golangci/golangci-lint-action@v2 -> golangci/golangci-lint-action@v3

@pedreviljoen Let us know if you are still interested in finishing this.

@pedreviljoen
Copy link
Contributor Author

@andrerfcsantos
Ok cool will do so tomorrow, while doing the upgrade to 1.18 I see there are two other open requests too. Will perhaps do all 3 then 😅

@pedreviljoen
Copy link
Contributor Author

@andrerfcsantos
Implemented the recommended updates, let me know if you need anything else?

@pedreviljoen pedreviljoen requested a review from junedev May 12, 2022 12:23
Copy link
Member

@andrerfcsantos andrerfcsantos left a comment

Choose a reason for hiding this comment

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

@pedreviljoen The tests for the repo are now passing, which is great! This is definitively an improvement.

My only suggestion would only be to convert all actions/setup-go@v2 to actions/setup-go@v3 not just the one for the "tests" job - sorry if I didn't make it clear earlier. In addition, convert all instances of actions/checkout@v2 to actions/checkout@v3 - just noticed they also have a v3 now.

@andrerfcsantos
Copy link
Member

andrerfcsantos commented May 12, 2022

@pedreviljoen We'll gladly let you take care of the upgrades for the other repos too, thanks for the help!

Those upgrades should be similar to this one, where we should also upgrade actions/setup-go, actions/checkout and golangci/golangci-lint-action there too where applicable.

The only difference to this PR that comes to mind is that in the main repo (exercism/go), we are installing golangci-lint via a script and not an action, so there the version of golangci-lint must be changed by changing the URL in the script and not by changing an action configuration. And bumping the version of go.mod files that are part of tests is ok, but we should not do it for the go.mod files of student's exercises, specially if the exercise doesn't require generics or anything else in 1.18.

@andrerfcsantos
Copy link
Member

@pedreviljoen Still interested in working on this? Totally ok if you are not, just let us know and we can finish it for you. Otherwise, we are happy to let you finish.

@pedreviljoen
Copy link
Contributor Author

@andrerfcsantos
Thanks for pinging me, got busy with a project at work. Will look into the mentioned suggestions and feed back 👍

Copy link
Member

@andrerfcsantos andrerfcsantos left a comment

Choose a reason for hiding this comment

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

Seems like the pipeline is failing now on gcov2lcov. My suggestion would be to upgrade those versions too to see if the problem is fixed:

  • jandelgado/gcov2lcov-action@v1.0.6 -> jandelgado/gcov2lcov-action@v1.0.9
  • coverallsapp/github-action@v1.1.2 -> coverallsapp/github-action@v1.1.3

@pedreviljoen
Copy link
Contributor Author

@andrerfcsantos
All good, updated those coverage jobs failing

.github/workflows/test.yml Outdated Show resolved Hide resolved
@andrerfcsantos
Copy link
Member

That seems to have fixed it, cool!

Just waiting for more approvals and for the other tooling PRs before we can merge this one.

Having also the upgrade of the analyzer to 1.18 at least would be important for us to say that the track supports 1.18. Would you also like to work on that one?

@pedreviljoen
Copy link
Contributor Author

@andrerfcsantos
yeah cool, will do 👍

@pedreviljoen
Copy link
Contributor Author

@andrerfcsantos
I think we are in a good place now?

analyzer and representer also bumped to 1.18

@andrerfcsantos
Copy link
Member

Yeah. We now just need a review from @exercism/maintainers-admin

@ErikSchierboom
Copy link
Member

Is Go 1.18 backwards compatible with the currently used version (1.17 I think)?

@andrerfcsantos
Copy link
Member

@ErikSchierboom Yes. New 1.x releases have the implicit promise of being compatible with all previous 1.x versions of Go. This is known as the "Go 1 compatibility promise", and it's detailed here Go 1 and the Future of Go Programs
.

It is intended that programs written to the Go 1 specification will continue to compile and run correctly, unchanged, over the lifetime of that specification. At some indefinite point, a Go 2 specification may arise, but until that time, Go programs that work today should continue to work even as future "point" releases of Go 1 arise (Go 1.1, Go 1.2, etc.).

The other related PRs:

@@ -8,31 +8,31 @@ jobs:
runs-on: ubuntu-latest
steps:
- name: Install Go
uses: actions/setup-go@v2
uses: actions/setup-go@v3
Copy link
Member

Choose a reason for hiding this comment

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

Our GHA recommendations guide has a section on pinning actions to SHAs, for security purposes. Admittedly, the actions/ actions are less likely to be affected, but this workflow also uses some workflows developed by third-parties, for which I'd definitely recommend pinning them. It's totally fine to do this in a follow-up PR.

@andrerfcsantos andrerfcsantos merged commit 1b711ce into exercism:main Jul 9, 2022
@andrerfcsantos
Copy link
Member

@ErikSchierboom I'll do follow-up PRs then :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants