-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Upgrade to go 1.22 #8125
Upgrade to go 1.22 #8125
Conversation
7b22c86
to
26f8c29
Compare
08c2a18
to
7e205f8
Compare
7e205f8
to
76e7b9f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Thank you @bryantbiggs for working on this! :) i left a couple comments in line
Makefile
Outdated
generate-always: ## Generate code (required for every build) | ||
go install github.com/maxbrunsfeld/counterfeiter/v6 | ||
go install github.com/vektra/mockery/v2 | ||
go install github.com/vburenin/ifacemaker |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super nit: we can move the go installs under a differ make rule and just call it here:
generate-always: ## Generate code (required for every build) | |
go install github.com/maxbrunsfeld/counterfeiter/v6 | |
go install github.com/vektra/mockery/v2 | |
go install github.com/vburenin/ifacemaker | |
generate-always: install-gen-tools ## Generate code (required for every build) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good call - added a new target for installing tools 3331498
should this target run automatically where tools are required, or should that be an explicit step that CI/users take on their own
Makefile
Outdated
unit-test-no-generate: | ||
go install github.com/cloudflare/cfssl/cmd/...@latest | ||
CGO_ENABLED=0 go test -tags=release ./pkg/... ./cmd/... $(UNIT_TEST_ARGS) | ||
|
||
.PHONY: unit-test-race | ||
unit-test-race: ## Run unit test with race detection | ||
go install github.com/cloudflare/cfssl/cmd/...@latest | ||
CGO_ENABLED=1 go test -race ./pkg/... ./cmd/... $(UNIT_TEST_ARGS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is cfssl
binary being called inside the unit tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately, yes
eksctl/pkg/iam/oidc/testdata/Makefile
Line 15 in 83e5ccc
cfssl gencert \ |
the unit tests are not "pure" unit tests - for example, they reach out to ECR to pull images
bf36726
to
181d22e
Compare
181d22e
to
12bc753
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
This reverts commit ccdd44c.
) * Revert "Bump github.com/gofrs/flock from 0.8.1 to 0.12.1 (#8024)" This reverts commit 18394bd. * Revert "Bump github.com/github-release/github-release from 0.10.0 to 0.10.1 (#8107)" This reverts commit 34ab1f8. * Revert "Migrate `goformation` under `pkg/` as a local package and remove location re-write (#8153)" This reverts commit d0e5360. * Revert "Upgrade to go 1.22 (#8125)" This reverts commit ccdd44c.
Description
1.21 reached end of support in August of 2024.
1.22 was chosen because there were errors thrown on 1.23 - tabled for later date.
This PR is a bit larger than usual given the way the prior Docker images were configured in order to update the go version. Previously, it appears that you would need to perform the following to update the go version:
eksctl
"build" image to ECR. This means that only maintainers with write access to ECR can update go versions, and this is done locally! Not greatThis is interesting for two reasons:
eksctl
binary is copied out of this dev build image into the resulting final image. If this psuedo development environment image was of value, there are probably better ways to achieve this using multi-stage builds and removing the need to have a person push the intermediate image to ECR from their local machineTo simplify, this PR removes the dev build image entirely which removes the majority of the complexity in this process. The resulting image that is pushed to ECR is based on other K8s/EKS images. I did not get a chance to dig into the code generation and whether any of that is used in the resulting final executable or if that is only for testing, but there could be further simplification - tabled for a later date.
With this changes, we therefore can remove the following:
.requirements
file is removed which avoids version skew issues; any binaries needed are instead installed viago install ...
in the Makefile before their useinstall-build-deps
Make targeteksctl-build
image, scripts, and manifests are removed.github/workflows/ecr-publish-build.yaml
workflowMakefile.docker
.github/workflows/build-all-distros-nightly.yaml
workflow is removed since its redundant of the CI checks, and has been failing for some time.github/workflows/cache-dependencies.yaml
workflow is removed since there are already a number of cache points in the workflows and we are over-caching which is rendering this mootWith this change, an outside OSS contributor can now update the version go and build, test, and validate that change before creating a PR without any maintainer involvement.
Checklist
README.md
, or theuserdocs
directory)area/nodegroup
) and kind (e.g.kind/improvement
)BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯