Add capability to build docker all-in-one #3813
Conversation
Signed-off-by: Albert Teoh <[email protected]>
| --build-arg base_image=$(BASE_IMAGE) \ | ||
| --build-arg debug_image=$(DEBUG_IMAGE) \ | ||
| --build-arg TARGETARCH=$(GOARCH) \ | ||
| --load \ |
There was a problem hiding this comment.
Necessary for registering the images into docker so they are visible via docker images.
Makefile
Outdated
| # Save the current Internal Field Separator (IFS) for later recovery, then set it to a comma-separator. | ||
| # The reason for using a comma-separator is to support the empty prefix required for all-in-one. | ||
| OLDIFS=$IFS; \ | ||
| IFS=","; \ | ||
| for pair in "jaeger,agent" "jaeger,collector" "jaeger,query" "jaeger,ingester" ",all-in-one" ; do \ |
There was a problem hiding this comment.
Open to other suggestions on a better approach.
There was a problem hiding this comment.
perhaps it's simpler to use strings like jaeger-agent and then just regex-out the jaeger- prefix when referring to the working dir cmd/$$component
There was a problem hiding this comment.
Good suggestion, done in: 353cc39
Codecov Report
@@ Coverage Diff @@
## main #3813 +/- ##
==========================================
+ Coverage 97.58% 97.59% +0.01%
==========================================
Files 289 289
Lines 16800 16800
==========================================
+ Hits 16394 16396 +2
+ Misses 321 319 -2
Partials 85 85
Continue to review full report at Codecov.
|
Signed-off-by: Albert Teoh <[email protected]>
Makefile
Outdated
| component_suffix="$${BASH_REMATCH[2]}"; \ | ||
| fi; \ | ||
| docker buildx build --target $(TARGET) \ | ||
| --tag $(DOCKER_NAMESPACE)/$$component_prefix$$component_suffix$(SUFFIX):${DOCKER_TAG} \ |
There was a problem hiding this comment.
| --tag $(DOCKER_NAMESPACE)/$$component_prefix$$component_suffix$(SUFFIX):${DOCKER_TAG} \ | |
| --tag $(DOCKER_NAMESPACE)/$$component$(SUFFIX):${DOCKER_TAG} \ |
Makefile
Outdated
| done | ||
| --load \ | ||
| cmd/$$component_suffix ; \ | ||
| echo "Finished building $$component_suffix ==============" ; \ |
There was a problem hiding this comment.
| echo "Finished building $$component_suffix ==============" ; \ | |
| echo "Finished building $$component ==============" ; \ |
Makefile
Outdated
| --tag $(DOCKER_NAMESPACE)/jaeger-$$component$(SUFFIX):${DOCKER_TAG} \ | ||
| for component in "jaeger-agent" "jaeger-collector" "jaeger-query" "jaeger-ingester" "all-in-one" ; do \ | ||
| regex="(jaeger-)(.*)"; \ | ||
| component_prefix=""; \ |
There was a problem hiding this comment.
Thanks, that's simpler: ae6b0e1
Signed-off-by: Albert Teoh <[email protected]>
Makefile
Outdated
| echo "Finished building $$component ==============" ; \ | ||
| done | ||
| done; \ |
There was a problem hiding this comment.
why does this have \ continuation?
There was a problem hiding this comment.
It was a remnant of the previous implementation where the IFS needed to be reset. Removed in c649fa6
| @@ -1,4 +1,5 @@ | |||
| JAEGER_IMPORT_PATH=github.com/jaegertracing/jaeger | |||
| SHELL := /bin/bash | |||
There was a problem hiding this comment.
Yes, because regex matches via the =~ operator requires [[ ... ]], which is only supported in bash, and not sh. Without it, the following error will result: /bin/sh: 4: [[: not found.
Signed-off-by: Albert Teoh <[email protected]>
|
Thanks for the review! |
Add `all-in-one` component to the list of components to build docker images for. Signed-off-by: Albert Teoh <[email protected]>
Which problem is this PR solving?
jaegertracing/all-in-one:latestimage.Short description of the changes
all-in-onecomponent to the list of components to build docker images for.scripts/build-all-in-one-image.shfor all-in-one, andscripts/build-upload-docker-images.shfor other components. These scripts, in turn, execute their own docker commands (not sure why, instead of using Makefile targets).Testing