run in docker cleanup and fixes #2623
Conversation
eks-distro-bot
added
the
size/M
| CURRENT_BUILDER_BASE_TAG=$(or \ | ||
| $(and $(wildcard /config/BUILDER_BASE_TAG_FILE),$(shell cat /config/BUILDER_BASE_TAG_FILE))\ | ||
| ,$(shell curl -s https://raw.githubusercontent.com/aws/eks-anywhere-prow-jobs/main/BUILDER_BASE_TAG_FILE)) | ||
| CURRENT_BUILDER_BASE_IMAGE=$(if $(CODEBUILD_BUILD_IMAGE),$(CODEBUILD_BUILD_IMAGE),$(BASE_IMAGE_REPO)/builder-base:$(CURRENT_BUILDER_BASE_TAG)) | ||
| GOLANG_GCC_BUILDER_IMAGE=$(BASE_IMAGE_REPO)/golang:$(shell cat $(BASE_DIRECTORY)/EKS_DISTRO_MINIMAL_BASE_GOLANG_COMPILER_$(GOLANG_VERSION)_GCC_TAG_FILE) | ||
|
|
||
| # in CODEBUILD always use buildctl | ||
| BUILDCTL_AVAILABLE=$(or $(filter true,$(CODEBUILD_CI)),$(shell command -v buildctl &> /dev/null && buildctl debug workers &> /dev/null && echo "true" || echo "false")) | ||
| BUILDX_AVAILABLE=$(shell docker buildx inspect &> /dev/null && echo "true" || echo "false") | ||
| BUILDCTL_AVAILABLE=$(or $(filter true,$(IS_ON_BUILDER_BASE)),$(shell command -v buildctl &> /dev/null && buildctl debug workers &> /dev/null && echo "true" || echo "false")) |
There was a problem hiding this comment.
changing this so that if u happen to run the builder base locally and want to build images, itll do what we do in codebuiler, which is launch buildkitd on the fy.
| @@ -1050,7 +1057,7 @@ ensure-docker: ensure/docker | |||
| # in that case skip this check | |||
| .PHONY: ensure-buildkitd-host | |||
| ensure-buildkitd-host: | ensure-buildctl | |||
| @if [ "true" = "$(CODEBUILD_CI)" ] && [ "true" = "$(IS_ON_BUILDER_BASE)" ]; then \ | |||
| @if [ "true" = "$(IS_ON_BUILDER_BASE)" ]; then \ | |||
| $(_TARGET): SHELL=$(if $(_USE_DOCKER),$(DOCKER_SHELL),$(LOGGING_SHELL)) | ||
| endef | ||
|
|
||
| define RUN_IN_DOCKER_ARGS_BODY |
There was a problem hiding this comment.
broke this out to support using it in the actual target, in case someone wants to run it manually, and to export it for the implicit target.
| @@ -53,14 +53,14 @@ else | |||
| trap "remove_container" EXIT | |||
| fi | |||
|
|
|||
| IMAGE="public.ecr.aws/eks-distro-build-tooling/builder-base:$BUILDER_BASE_TAG" | |||
| IMAGE="736510011942.dkr.ecr.us-west-2.amazonaws.com/builder-base:latest.2" | |||
| @@ -92,11 +92,16 @@ if [[ "$SKIP_RUN" == "false" ]]; then | |||
| DOCKER_RUN_NETRC="" | |||
| fi | |||
|
|
|||
| PRIVILEGED="" | |||
| if [[ "$REMOVE" == "false" ]]; then | |||
| PRIVILEGED="--privileged --mount type=bind,source=/var/run/docker.sock,target=/var/run/docker.sock" | |||
There was a problem hiding this comment.
i dont think we need to run as privileged, i have tried building on al2 and mac and it seems fine to not include this flag. only including it for the case that someone uses start-builder target to start the long lived container
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhay-krishna 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 |
Issue #, if available:
Description of changes:
Cleanups post my last PR enable docker in most targets.
This actually does change the docker strategy from using the "fake" prereq target to manipulating the shell instead, similar to the way we do loggging. This appears to work out better both on my mac and al2 box. One of the issues this fixes is where the docker targets were being run even when the file didnt actually need updating.
The approach taken is to export RUN_IN_DOCKER_ARGS which will only be exported on the shell for the actual target body, not any other $(shell) calls that happen while generating the body, which is important since we rely on that to know when to run the passed in args vs the export docker args.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.