build: BPFMAN_IMG & BPFMAN_AGENT_IMG to overwrite image #18
Conversation
Billy99
force-pushed
the
billy99-kustomize-image
branch
from
5ab8047
to
6a0d6f8
Compare
|
To reproduce, run something like: Then use |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #18 +/- ##
=======================================
Coverage 26.38% 26.38%
=======================================
Files 75 75
Lines 5021 5021
=======================================
Hits 1325 1325
Misses 3532 3532
Partials 164 164
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
NOTE: |
Billy99
force-pushed
the
billy99-kustomize-image
branch
from
6a0d6f8
to
aa19fa7
Compare
The bpfman-operator is setup to allow BPFMAN_IMG to overwrite the default bpfman image, and BPFMAN_AGENT_IMG to overwrite the bpfman-agent image. However, the Makefile is leveraging kustomize. kustomize can replace an image string in a yaml when it knows the k8s object layout, but these images are passed via a ConfigMap, which is opaque data. So the current implementation doesn't work. kustomize does have a ConfigMapGenrator, which can replace the contents of a ConfigMap. The change is to: * Change the kustomization.yaml to use a `configMapGenerator`. * Use `sed` to replace the default images with those passed in (or just the default image if none were passed in). The `kustomize edit set image` command doesn't work. The `sed` command is changing the file content, so rename kustomization.yaml to kustomization.yaml.env and piped the changes to kustomization.yaml. * Add kustomization.yaml to .gitignore so changes aren't tracked by git. Signed-off-by: Billy McFall <22157057+Billy99@users.noreply.github.com>
| @@ -291,6 +291,10 @@ test: fmt envtest ## Run Unit tests. | |||
| .PHONY: test-integration | |||
| test-integration: ## Run Integration tests. | |||
| go clean -testcache | |||
| cd config/bpfman-deployment && \ | |||
| sed -e 's@bpfman\.image=.*@bpfman.image=$(BPFMAN_IMG)@' \ | |||
There was a problem hiding this comment.
u could use yq to replace in place the image as well and avoid the tmp kustomize ?
There was a problem hiding this comment.
Yes, we discussed yq in the past, but decided we didn't want to add another tool. We are already doing something similar in the examples Makefile.
There was a problem hiding this comment.
just FYI sed require some magic to work on mac os, probably something like
ifeq (,$(shell which gsed 2>/dev/null))
SED ?= sed
else
SED ?= gsed
endifthen use SED everywhere
There was a problem hiding this comment.
Good to know. I don't think anyone has tried to build/run on a MAC, but should probably be added to the list.
WDYT about the following to fix .PHONY: load-images-kind
load-images-kind: ## Load bpfman-agent, and bpfman-operator images into the running local kind devel cluster.
- kind load docker-image ${BPFMAN_OPERATOR_IMG} ${BPFMAN_AGENT_IMG} --name ${KIND_CLUSTER_NAME}
+ kind load docker-image ${BPFMAN_OPERATOR_IMG}-${MULTIARCH_TARGETS} ${BPFMAN_AGENT_IMG}-${MULTIARCH_TARGETS} --name ${KIND_CLUSTER_NAME} |
That's probably fine as long as that's the only place it is needed. |
|
/LGTM |
Red Hat Konflux update ocp-bpfman-agent
The bpfman-operator is setup to allow BPFMAN_IMG to overwrite the default bpfman image, and BPFMAN_AGENT_IMG to overwrite the bpfman-agent image. However, the Makefile is leveraging kustomize. kustomize can replace an image string in a yaml when it knows the k8s object layout, but these images are passed via a ConfigMap, which is opaque data. So the current implementation doesn't work. kustomize does have a ConfigMapGenrator, which can replace the contents of a ConfigMap.
The change is to:
configMapGenerator.sedto replace the default images with those passed in (or just the default image if none were passed in). Thekustomize edit set imagecommand doesn't work. Thesedcommand is changing the file content, so rename kustomization.yaml to kustomization.yaml.env and piped the changes to kustomization.yaml.