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

Remove explict go toolchain versions #5723

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

ddl-ebrown
Copy link
Contributor

@ddl-ebrown ddl-ebrown commented Sep 4, 2024

Tracking issue

Why are the changes needed?

  • [Housekeeping] Bump Go version to 1.22 #5032 updated builds to use Go 1.22 -- but in practice it looks like flytectl is being built with an outdated toolchain based on Go 1.22.0 instead of Go 1.22.5 or 1.22.6 (i.e. latest)

    For instance, flytectl v0.9.0 reports being built with Go 1.22.0

❯ go version -m -v
flytectl flytectl: go1.22.0

Ensure the proper Go environment for security reasons

What changes were proposed in this pull request?

How was this patch tested?

Setup process

Screenshots

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

Docs link

 - flyteorg#5032 updated builds to use
   Go 1.22 -- but in practice it looks like flytectl is being built with
   an outdated toolchain based on Go 1.22.0 instead of Go 1.22.5 or
   1.22.6 (i.e. latest)

   See https://go.dev/doc/toolchain for further details

   For instance, flytectl v0.9.0 reports being built with Go 1.22.0

   ❯ go version -m -v flytectl
   flytectl: go1.22.0

   Ensure the proper Go environment for security reasons

Signed-off-by: ddl-ebrown <[email protected]>
Copy link

codecov bot commented Sep 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 36.16%. Comparing base (d4db50c) to head (c94d2f4).
Report is 7 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5723      +/-   ##
==========================================
- Coverage   36.16%   36.16%   -0.01%     
==========================================
  Files        1303     1303              
  Lines      109672   109672              
==========================================
- Hits        39668    39665       -3     
- Misses      65858    65861       +3     
  Partials     4146     4146              
Flag Coverage Δ
unittests-datacatalog 51.37% <ø> (ø)
unittests-flyteadmin 55.25% <ø> (ø)
unittests-flytecopilot 12.17% <ø> (ø)
unittests-flytectl 62.17% <ø> (-0.05%) ⬇️
unittests-flyteidl 7.12% <ø> (ø)
unittests-flyteplugins 53.35% <ø> (ø)
unittests-flytepropeller 41.76% <ø> (ø)
unittests-flytestdlib 55.35% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ddl-ebrown
Copy link
Contributor Author

If I build source locally, I get my local 1.23.0 toolchain already

❯ make compile
go build -o bin/flytectl -ldflags="-s -w -X github.com/flyteorg/flyte/flytestdlib/version.Version=v0.9.1-40-gc94d2f47b -X github.com/flyteorg/flyte/flytestdlib/version.Build=c94d2f47b -X github.com/flyteorg/flyte/flytestdlib/version.BuildTime=2024-09-04" main.go
❯ go version -m -v bin/flytectl
bin/flytectl: go1.23.0

So I'm not sure this PR is the actual solution here b/c I didn't see how flytectl is being built / shipped.

@eapolinario
Copy link
Contributor

If I build source locally, I get my local 1.23.0 toolchain already

https://go.dev/doc/toolchain#select has the answer for this. It's kind of convoluted, but essentially, if GOTOOLCHAIN is set to auto and you have a lower version than the one set in the go.mod file, then the version set in the go.mod is installed and used to run the go command.

I verified this by running:

❯ docker run --rm -it --workdir /code --volume $(pwd):/code golang:1.21.12-bookworm bash
root@fff6de1b2bf3:/code# go version
go version go1.21.12 linux/arm64
root@fff6de1b2bf3:/code# GOTOOLCHAIN=auto make -C flytectl compile
make: Entering directory '/code/flytectl'
go build -o bin/flytectl -ldflags="-s -w -X github.com/flyteorg/flyte/flytestdlib/version.Version=v0.9.1-39-gd4db50cb2 -X github.com/flyteorg/flyte/flytestdlib/version.Build=d4db50cb2 -X github.com/flyteorg/flyte/flytestdlib/version.BuildTime=2024-09-04" main.go
go: downloading go1.22.0 (linux/arm64)
...
root@fff6de1b2bf3:/code# go version -m ./flytectl/bin/flytectl | head -n1
./flytectl/bin/flytectl: go1.22.0

That env var is set in the case of that gh action: https://github.com/flyteorg/flyte/actions/runs/10269861937/job/28416285221#step:3:42.

If you run the same code but using the golang:1.23.0-bookworm image you'll see that the version is set to go1.23.0.

how flytectl is being built / shipped

we use this gh action to release flytectl: https://github.com/flyteorg/flyte/blob/master/.github/workflows/flytectl-release.yml.

The last time we released flytectl: https://github.com/flyteorg/flyte/actions/runs/10269861937. Notice how we specify go 1.21 but somehow we end up with go 1.22.0 installed. Reading the code for that specific version of setup-go@v4 I don't see how we end up installing 1.22.0.

That said, I feel like removing toolchain is the right call. Also, we should probably switch to setting up the go version from the go.mod file (so that it's easier to maintain the right version across go.mod and gh workflows). wdyt, @ddl-ebrown ?

@eapolinario eapolinario enabled auto-merge (squash) September 10, 2024 19:07
@eapolinario eapolinario merged commit 86c63f7 into flyteorg:master Sep 10, 2024
54 of 55 checks passed
bgedik pushed a commit to bgedik/flyte that referenced this pull request Sep 12, 2024
- flyteorg#5032 updated builds to use
   Go 1.22 -- but in practice it looks like flytectl is being built with
   an outdated toolchain based on Go 1.22.0 instead of Go 1.22.5 or
   1.22.6 (i.e. latest)

   See https://go.dev/doc/toolchain for further details

   For instance, flytectl v0.9.0 reports being built with Go 1.22.0

   ❯ go version -m -v flytectl
   flytectl: go1.22.0

   Ensure the proper Go environment for security reasons

Signed-off-by: ddl-ebrown <[email protected]>
Signed-off-by: Bugra Gedik <[email protected]>
@ddl-ebrown ddl-ebrown deleted the build-using-latest-go branch November 1, 2024 04:57
@eapolinario
Copy link
Contributor

we should probably switch to setting up the go version from the go.mod file (so that it's easier to maintain the right version across go.mod and gh workflows).

Trying out this idea in #5736.

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.

2 participants