-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Optional podman support for running tests locally #9294
Conversation
@kirs: This issue is currently awaiting triage. If Ingress contributors determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Hi @kirs. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
INGRESS_VOLUME=$(mktemp -d) | ||
if [[ "$OSTYPE" == darwin* ]] && [[ "$RUNTIME" == podman ]]; then | ||
mkdir -p "tmp" | ||
INGRESS_VOLUME=$(pwd)/$(mktemp -d tmp/XXXXXX) |
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.
This dance is needed because /var/folders/<some-tmp-name>
created on Mac is not available on the VM managed by Podman. It has to be something in the local dir of the project.
29f35d9
to
5f0feb6
Compare
@kirs thank you for this. Its an awesome help. But is this agnostic to OS ? Does it currently only work on M1 Mac or Intel Mac ? Does it need any change to be used on Linux lpatop/dekstop ? |
besides @longwuyuan question (which I agree) it is great! Thanks for that! Let me know if you need someone to test this on a Mac Intel :D |
@longwuyuan there's nothing here that would be M1 or Intel specific. As long as Podman is compatible with both this should just work. Same for Linux, I expect it to work as is. @rikatz I'd appreciate if you could help to test this on Mac Intel just in case 🙏 |
@kirs thanks for updating. If you did test on linux, please confirm. Sorry for persisting because there are some nuances experienced earlier relating to running tests in VMs |
22e8e50
to
a4f511a
Compare
Tested on Linux:
|
a4f511a
to
25a6171
Compare
@kirs thank you very much for the test. |
@kirs CI failing. I can try to answer question if any. PTAL |
Thanks, this idea LGTM One question, do we need to cover this scenario in CI? For example, test it with Podman? |
I keep an eye on some of Podman's features and documentation, but I don't really use it, especially on macOS |
Original idea in this PR is not for CI. Just for local use. So ideally, CI should ignore this now. In future, if many other projects are happy for several months with podman, then its ok to explore CI with podman. So I think currently just need to adjust CI so that it does not fail on podman. Not even sure if the latest CI failure is because of this change. |
25a6171
to
40193ad
Compare
40193ad
to
be1d27a
Compare
-v "/var/run/docker.sock:/var/run/docker.sock" \ | ||
-v "${INGRESS_VOLUME}:/etc/ingress-controller/" \ | ||
-w "/go/src/${PKG}" \ | ||
${E2E_IMAGE} /bin/bash -c "${FLAGS}" |
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.
Had to rewrite this because of that extra flag for docker below.
c8fce85
to
64e7186
Compare
build/run-in-docker.sh
Outdated
args="$args -v /var/run/docker.sock:/var/run/docker.sock" | ||
fi | ||
|
||
args="$args -v "${INGRESS_VOLUME}:/etc/ingress-controller/" -w "/go/src/${PKG}"" |
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 think you could make this part of the args
initialization above. Maybe you wanted to keep the order of args unchanged? I'd think order does not matter.
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.
Thanks for pointing this out, updated!
64e7186
to
f9582f6
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ElvinEfendi, kirs The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Some environments don't allow Docker for Desktop for licensing concerns and use Podman instead.
This is an attempt to allow some optional support for Podman, at least for running local tests.
You can try it with
RUNTIME=podman build/run-in-docker.sh test/test-lua.sh
.I spoke to @ElvinEfendi mentioned that the team would be open to swapping docker for local development with podman completely, I might take an attempt at some point - but right now I believe this smaller diff can unblock us faster to run tests locally.