-
Notifications
You must be signed in to change notification settings - Fork 1.5k
chore: add podman support to Makefile #9363
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
base: main
Are you sure you want to change the base?
Conversation
|
/kind changelog-not-required |
Signed-off-by: iTrooz <[email protected]>
|
Just noting that docker cli via podman compatibility works in this repo with its buildx. Do not require docker desktop license |
Signed-off-by: iTrooz <[email protected]>
|
Ok, I figured out why Turns out docker does not use UID mapping by default, so files owned by e.g. 1000 on the host will still be owned by 1000 in the container, so you need In order to make both podman and docker work, I restored the user switch, and added the option I tested |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9363 +/- ##
=======================================
Coverage 59.89% 59.89%
=======================================
Files 383 383
Lines 34949 34949
=======================================
Hits 20934 20934
Misses 12470 12470
Partials 1545 1545 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| # docker (or podman) command. | ||
| ifneq (, $(shell which docker 2>/dev/null)) | ||
| DOCKER := docker |
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.
It's better to use another word instead of DOCKER.
Essentially, podman is different from docker.
Using DOCKER is misleading.
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.
Image tool, Container or builder should work
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 initially named it DOCKER because that command had to be compatible with the Docker CLI interface, but okay I'll make the change
| @echo creating a buildx instance | ||
| -docker buildx rm velero-builder || true | ||
| @docker buildx create --use --name=velero-builder | ||
| -$(DOCKER) buildx rm velero-builder || true |
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'm uncertain about this place.
Does podman support multiple platform builds the same way as docker?
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 found something here.
It seems docker and podman work differently.
how to build multi-architeture container images
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.
Some commands are the same some are not.
|
Hum, the podman CLI does not seem to be fully equivalent to docker's, after all
These problems all arise on the As for platform builds, it seems podman supports the same All in all, this PR will be a little more complicated than just adding a variable and calling it a day. As such, I'd understand if you preferred to close it instead. If not, I am still ready to work on this in the next few days |
|
Here's the main quirk you should be aware of. |
Thank you for contributing to Velero!
Please add a summary of your change
This PR adds support for using the "podman" CLI executable instead of docker
I did 2 things:
dockerCLI invocations with the$(DOCKER)variable, defined at the top of the file- removed the-u $$(id -u):$$(id -g)of your docker run shell creation. I'm not sure if it was useful before, but my tests work fine without it.I tested my changes on Linux, with both
podmananddocker, on targetslocal,testandupdateDoes your change fix a particular issue?
No
Please indicate you've done the following:
make new-changelog) or comment/kind changelog-not-requiredon this PR.site/content/docs/main.