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

Repo controlled build go version #206

Merged
merged 2 commits into from
Jan 24, 2024
Merged

Repo controlled build go version #206

merged 2 commits into from
Jan 24, 2024

Conversation

xdu31
Copy link
Member

@xdu31 xdu31 commented Jan 23, 2024

Issue #, if available:

Description of changes:

Follow kubernetes/enhancements#3744 to make amazon-eks-pod-identity-webhook to stay on supported Go versions.

This PR follows kubernetes/kubernetes#114660 and kubernetes/kubernetes#115377 for repo-controlled go version to use in CI.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@xdu31 xdu31 requested a review from a team as a code owner January 23, 2024 15:50
@@ -0,0 +1,6 @@
# gimme
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add why do we need to include gimme to the repo?
I am not familiar with this, if you can some pointers it will be helpful.

Copy link
Member Author

Choose a reason for hiding this comment

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

Responded with details in another thread: #206 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a binary?

Copy link
Member Author

@xdu31 xdu31 Jan 23, 2024

Choose a reason for hiding this comment

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

This is a shell script: https://github.com/travis-ci/gimme

Responded with details in another thread: #206 (comment)

fi

# force go modules
export GO111MODULE=on
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't Go modules enabled by default?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is to make sure we force it on, in case it is disabled, and this setup-go.sh was taken from kubernetes-sigs/kind, which was later adopted in kubernetes repository as part of the work to keep Kubernetes on supported Go versions.

The gimme script has the ability to install temporarily in local environment with the exact specified Go version without interruption local Go installation. With this ability installed in a repo, when we are running any unit tests or integration tests, we can make sure we are running with the exact specified Go version from .go-version file.

See kubernetes adoption on gimme in: kubernetes/kubernetes#115377

See the initial adoption of gimme in kind in: https://github.com/kubernetes-sigs/kind/blob/main/hack/build/setup-go.sh

hack/setup-go.sh Outdated
Comment on lines 34 to 39
# go version go1.14.5 darwin/amd64
if ! ([ -n "${FORCE_HOST_GO:-}" ] || \
(command -v go >/dev/null && [ "$(go version | cut -d' ' -f3)" = "go${GO_VERSION}" ])); then
# eval because the output of this is shell to set PATH etc.
eval "$(hack/third_party/gimme/gimme "${GO_VERSION}")"
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this to update Go version when building locally? If yes, I would keep the build option same docker build because we don't know how Go is being setup locally.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @prateekgogia We do keep the same option for docker build by:

.image-linux-%:
	docker buildx build --output=type=docker --platform linux/$* \
		--build-arg golang_image=$(shell hack/setup-go.sh) --no-cache \
		--tag $(IMAGE):$(GIT_COMMIT)-linux_$* .

Here we explicitly call setup-go.sh with build-arg:

--build-arg golang_image=$(shell hack/setup-go.sh) --no-cache \

In this way, we are able to use exactly the Go image as base image with exactly the same version specified in .go-version file

This linked code change is in:
https://github.com/aws/amazon-eks-pod-identity-webhook/pull/206/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52R38

Copy link
Member Author

@xdu31 xdu31 Jan 23, 2024

Choose a reason for hiding this comment

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

Basically, this PR will make sure, when we are in:
build binaries
do unit tests
build images
github workflow CIs

We are using the exact specified Go version from .go-version in this repo, we call this repo controlled Go version setup.

Dockerfile Show resolved Hide resolved
@xdu31 xdu31 requested a review from prateekgogia January 23, 2024 19:49
Dockerfile Outdated
@@ -1,7 +1,9 @@
FROM --platform=$BUILDPLATFORM golang:1.21 AS builder
ARG golang_image=golang:1.21
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use this image here public.ecr.aws/eks-distro-build-tooling/golang?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

Makefile Outdated

# Generic make
REGISTRY_ID?=602401143452
REPO?=gcr.io/must-override
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC, we don't store images in gcr anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@xdu31 xdu31 requested a review from prateekgogia January 24, 2024 06:36

# Generic make
REGISTRY_ID?=602401143452
REGISTRY?=public.ecr.aws
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we already push this image to this repo?

Copy link
Member Author

@xdu31 xdu31 Jan 24, 2024

Choose a reason for hiding this comment

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

this is just a placeholder for override, for local docker build, it doesn’t matter

When an external user wants to build his own image and push it, he can do:
IMAGE=xxx make image
then do a docker push

@xdu31 xdu31 merged commit 33a1516 into aws:master Jan 24, 2024
1 check passed
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