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

[Doc] Mention minimum Go version required for development #1288

Merged
merged 4 commits into from
Dec 4, 2023

Conversation

kevin85421
Copy link
Contributor

@kevin85421 kevin85421 commented Nov 30, 2023

Proposed changes

My devbox uses Go 1.20.11 by default, so I got some compilation errors when I ran make build. In addition, go.mod requires 1.21.3, but the doc does not mention it.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

Screen Shot 2023-11-29 at 10 30 07 PM

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Nov 30, 2023
@kevin85421 kevin85421 marked this pull request as ready for review November 30, 2023 06:33
@kevin85421 kevin85421 requested a review from a team as a code owner November 30, 2023 06:33
@lucacome
Copy link
Member

lucacome commented Nov 30, 2023

@kevin85421 I think we should say that the project requires the version to be at least 1.21. And maybe we could add a note about the go toolchain.
Setting GOTOOLCHAIN=auto will make go find the appropriate toolchain, so even if you're on 1.21.2 and the go.mod says 1.21.4 you'll still be able to build

@kevin85421
Copy link
Contributor Author

I think we should say that the project requires the version to be at least 1.21. And maybe we could add a note about the go toolchain.

This makes sense to me. Based on information from https://go.dev/doc/toolchain, starting with Go 1.21, support for GOTOOLCHAIN is provided. Therefore, it will compare the version of the bundled toolchain with the versions specified by the go or toolchain lines in go.mod or go.work. If GOTOOLCHAIN=auto is set, it will select the newer version.

Setting GOTOOLCHAIN=auto will make go find the appropriate toolchain, so even if you're on 1.21.2 and the go.mod says 1.21.4 you'll still be able to build

Do we need to set GOTOOLCHAIN=auto explicitly? The toolchain doc says that the default GOTOOLCHAIN setting is auto.

I experimented as shown in the following screenshot:

  • GOTOOLCHAIN starts from Go 1.21. Hence, when I use Go 1.20.11 under the nginx-gateway-fabric directory, it does not switch to the newer toolchain (Go 1.21.3 for this case) specified by go.mod under the directory.
  • When I use Go 1.21.2 in a directory without a go.mod file, which is shown as workspace in the screenshot, Go 1.21.2 is used. However, when I operate under the nginx-gateway-fabric directory, it automatically switches to Go 1.21.3.

Screen Shot 2023-12-01 at 8 59 13 AM

Several questions

  • Should I say "latest version of Go" or "at least 1.21"?
  • Do I need to set GOTOOLCHAIN=auto explicitly?

Thanks!

@sjberman
Copy link
Contributor

sjberman commented Dec 1, 2023

@kevin85421 I'd say "at least Go v1.21". GOTOOLCHAIN=auto is the default, so we may not need to mention it.

Copy link

netlify bot commented Dec 2, 2023

Deploy Preview for nginx-gateway-fabric ready!

Name Link
🔨 Latest commit c7972d2
🔍 Latest deploy log https://app.netlify.com/sites/nginx-gateway-fabric/deploys/656e10071da2af00088d6b19
😎 Deploy Preview https://deploy-preview-1288--nginx-gateway-fabric.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@kevin85421
Copy link
Contributor Author

@kevin85421 I'd say "at least Go v1.21". GOTOOLCHAIN=auto is the default, so we may not need to mention it.

Thanks! I've already updated the document. I retained the Go installation command because https://go.dev/doc/install advises users to "Remove any previous Go installation by deleting the /usr/local/go folder".

@sjberman
Copy link
Contributor

sjberman commented Dec 4, 2023

I don't think we need the example instructions on how to install Go. Their docs should explain that. You're also using Go to install Go, so that's a little confusing. I would just keep to the simple link and and version.

@kevin85421
Copy link
Contributor Author

I don't think we need the example instructions on how to install Go. Their docs should explain that. You're also using Go to install Go, so that's a little confusing. I would just keep to the simple link and and version.

This makes sense. I have updated the PR. Thanks!

@sjberman sjberman changed the title [Doc] Add installation instructions for Go 1.21.3 [Doc] Mention minimum Go version required Dec 4, 2023
@sjberman sjberman changed the title [Doc] Mention minimum Go version required [Doc] Mention minimum Go version required for development Dec 4, 2023
@sjberman sjberman enabled auto-merge (squash) December 4, 2023 17:45
@sjberman sjberman merged commit d3313ac into nginxinc:main Dec 4, 2023
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants