-
Notifications
You must be signed in to change notification settings - Fork 82
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
refactor(e2e): use consistent tag name when building images at the same time #852
base: main
Are you sure you want to change the base?
Conversation
c3bca8e
to
e1c3edd
Compare
e1c3edd
to
c511669
Compare
c511669
to
1c07958
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.
Added some suggestions.
ifeq ($(origin TAG_NAME), undefined) | ||
TAG_NAME:=$(shell date "+gmp-%Y%d%m_%H%M%S_%N") | ||
endif | ||
export TAG_NAME |
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.
Can we just do this?
BUILD_DATE:=$(shell date "+%Y%d%m_%H%M%S_%N")
export TAG_NAME?=gmp-${BUILD_DATE}
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.
I believe so. They point out it is equivalent in: https://www.gnu.org/software/make/manual/html_node/Setting.html
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.
I investigated this and they're not 1:1.
With yours, we will always evaluate BUILD_TIME
. If someone uses it and calls $(MAKE)
, the value will be different in each execution in each target.
The reason why mine works is because my if statement says if the variable is not set, then set it. In your example, TAG_NAME
is lazily evaluated which also means you also can't simply as export TAG_NAME?=gmp-$(shell date "+%Y%d%m_%H%M%S_%N")
, since it's re-evaluated each time you call $(MAKE)
. While your version is simpler I feel it's more error-prone in these 2 ways.
As such, I kept my original version. Any strong thoughts here?
Makefile
Outdated
@echo ">> tagging and pushing images" | ||
$(call docker_tag_push,gmp/$(BIN_GO_NAME),${IMAGE_REGISTRY}/$(BIN_GO_NAME):${TAG_NAME}) | ||
$(call docker_tag, "gmp/$(BIN_GO_NAME):$(TAG_NAME)" "gmp/$(BIN_GO_NAME):latest") | ||
$(call docker_tag, "gmp/$(BIN_GO_NAME):$(TAG_NAME)" "${IMAGE_REGISTRY}/$(BIN_GO_NAME):$(TAG_NAME)") |
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.
Suggestion: It's possible to do multiple tags with one build command. No need to call tag
separately for this case.
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 docker build
. docker tag
does not support multiple tags.
1c07958
to
814c584
Compare
814c584
to
a1c775d
Compare
tl;dr: this ensures that when you run
make e2e
, all Docker images are built with the same tag.Another try for #845 (thanks @pintohutch)
How this works
$(MAKE)
which calls and forksmake
without carrying variables over. This is fixed by usingexport
.make
has rules for variable expansion.=
and?=
are considered "recursively expansions" which you can think of as lazily evaluated.:=
is considered "simply expanded" and is evaluated at declaration time. This is what we want to ensure it's only evaluated once.e2e-only
target which does not rebuild all images. This needs to be conditional so I'll fix this in a separate PR.