-
Notifications
You must be signed in to change notification settings - Fork 190
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
Add multi-arch and automatic Go version build support #391
Conversation
/hold |
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.
Looks good! I have two questions
SERVICE_CONTROLLER_GIT_VERSION=$(git describe --tags --always --dirty || echo "unknown") | ||
SERVICE_CONTROLLER_GIT_COMMIT=$(git rev-parse HEAD) |
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.
Ah, i guess this is shellcheck recommendation?
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.
Yes. Using backticks to run commands is not recommended
@@ -79,6 +88,11 @@ if ! is_public_ecr_logged_in; then | |||
aws ecr-public get-login-password --region us-east-1 | docker login --username AWS --password-stdin public.ecr.aws | |||
fi | |||
|
|||
pushd "$ROOT_DIR" 1>/dev/null | |||
# Get the golang version from the code-generator | |||
GOLANG_VERSION=${GOLANG_VERSION:-"$(go list -f {{.GoVersion}} -m)"} |
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.
So if i understand well, now controllers will be built using a Go compiler version parsed from code-generator go.mod
? Meaning that if we bump the version in the go.mod, it will bump the compiler version?
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.
That's correct. We need to be using the minimum Golang version as the code-generator, which should also be the same minimum version as the runtime.
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.
/lgtm
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.
Diggin' these changes, thank you @RedbackThomson!
ARG base_image=public.ecr.aws/eks-distro-build-tooling/eks-distro-minimal-base-nonroot:2021-12-01-1638322424 | ||
|
||
# Golang image to use for compiling the manager | ||
ARG builder_image=public.ecr.aws/docker/library/golang |
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.
cool that we're updating to the AWS-maintained golang. ++
/retest |
/unhold |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: A-Hilaly, jaypipes, jljaco, RedbackThomson The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@RedbackThomson: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Description of changes: When adding the [Go version and architecture to the code-generator Dockerfile](aws-controllers-k8s/code-generator#391), it broke the continuous deployment scripts which use a separate set of calls (to `buildah`) instead of using `build-controller-image.sh`. This pull request adds the same `GOARCH` and `GOLANG_VERSION` build arguments to the `release-controller.sh` script and adds the Go CLI to the deploy container to support them. These changes were validated by running a custom edit of the Prowjob against the new images and script. Log output: https://prow.ack.aws.dev/view/s3/ack-prow-logs/logs/lambda-post-submit/1620583281521070080 By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Extends #358
Description of changes:
Adds support for
GOARCH
in the controller image Dockerfile and relevant scripts. Updates the Dockerfile to use the Go version specified ingo.mod
file (upgraded to 1.19).By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.