🐛 Sanitize temporary filename for podman image load #803
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #803 +/- ##
=======================================
Coverage 67.21% 67.21%
=======================================
Files 23 23
Lines 1467 1467
=======================================
Hits 986 986
Misses 415 415
Partials 66 66
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Makefile
Outdated
| rm $(IMG).tar | ||
| $(CONTAINER_RUNTIME) save $(IMG) -o /tmp/$(shell echo $(IMG) | sed 's/[/:]/-/g').tar | ||
| $(KIND) load image-archive /tmp/$(shell echo $(IMG) | sed 's/[/:]/-/g').tar --name $(KIND_CLUSTER_NAME) | ||
| rm /tmp/$(shell echo $(IMG) | sed 's/[/:]/-/g').tar |
There was a problem hiding this comment.
Can we use mktemp here? I think it will be a bit more readable.
There was a problem hiding this comment.
Just with the default randomly generated filename to simplify all of this?
There was a problem hiding this comment.
Or even just a shell variable for the filename.
There was a problem hiding this comment.
Ahh, yeah good idea lol
Makefile
Outdated
| @@ -156,9 +156,9 @@ e2e-coverage: | |||
| kind-load: $(KIND) #EXHELP Loads the currently constructed image onto the cluster. | |||
| ifeq ($(CONTAINER_RUNTIME),podman) | |||
There was a problem hiding this comment.
Probably out of scope for what this PR is trying to accomplish (and I asked in the other now-closed PR), but would be possible to use this podman procedure for docker as well? That way we don't have to maintain two different implementations of kind-load.
There was a problem hiding this comment.
I would be find doing that, I can just remove the ifeq bits and leave only the first block with the temp archive. That should always work regardless of if the user is using podman, docker, or podman with the "podman-docker" alias package.
trgeiger
force-pushed
the
makefile-tmp-file
branch
from
62017c9
to
5ee07f7
Compare
Makefile
Outdated
| TMP_FILE=$(shell mktemp -t operator-controller-image-archive-XXXX.tar) ;\ | ||
| $(CONTAINER_RUNTIME) save $(IMG) -o $$TMP_FILE ;\ | ||
| $(KIND) load image-archive $$TMP_FILE --name $(KIND_CLUSTER_NAME) ;\ | ||
| rm $$TMP_FILE |
There was a problem hiding this comment.
Even simpler?
$(CONTAINER_RUNTIME) save $(IMG) | $(KIND) load image-archive /dev/stdin --name $(KIND_CLUSTER_NAME)
There was a problem hiding this comment.
very nice, implemented in latest push
There was a problem hiding this comment.
If you or someone else has docker available, would be nice just to double check this works as expected
There was a problem hiding this comment.
oh but the CI probably uses docker doesn't it?
This simplified logic should work on both podman and docker, removing our need to have distinct ways of handling the image loading.
trgeiger
force-pushed
the
makefile-tmp-file
branch
from
5ee07f7
to
d813099
Compare
Description
The podman-specific kind-load commands in the Makefile currently write the image archive to a temporary file named $IM but that by default includes slashes and a colon. My changes replace any slashes or colons with hyphens to prevent any errors trying to write that file.
Also, the naming doesn't really matter, we can just make it be a hard-coded "tmp.tar" or something.
Also, also, I saw the closed PR from earlier today and the discussion about not getting too into the weeds on podman/docker-specific configuration in the Makefile, but the current setup will cause that file-write error for anyone who does use CONTAINER_RUNTIME=podman with our default env variables, so I figured that was worth a quick fix.
Reviewer Checklist