-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Automatically execute podman-remote if vars set #4981
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: afbjorklund The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @afbjorklund. Thanks for your PR. I'm waiting for a containers 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. |
Hmm, think I messed up the error reporting when I made it into a function: $ ./bin/podman run busybox true
$ ./bin/podman run busybox false
Error running /usr/local/bin/podman-remote: exit status 1 |
263432b
to
7dbfbfb
Compare
If any of PODMAN_HOST or PODMAN_VARLINK_BRIDGE are set, then execute "podman-remote" rather than this "podman". Also support PODMAN_VARLINK_ADDRESS for tunneling, even though there is no encryption support in such addresses. Signed-off-by: Anders F Björklund <[email protected]>
7dbfbfb
to
4d097d1
Compare
Forgot PODMAN_VARLINK_ADDRESS, which was used in the documentation... |
/ok-to-test |
bridge := os.Getenv("PODMAN_VARLINK_BRIDGE") | ||
address := os.Getenv("PODMAN_VARLINK_ADDRESS") | ||
if host != "" || bridge != "" || address != "" { | ||
program := "podman-remote" |
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 might be good to have a debug statement in here to just say "Attempting to connect to remote Podman" or some such. I could see it as being potentially helpful in a debugging situation later on.
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.
So something like a logrus.Debugf
with the command being executed perhaps.
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.
Tried adding logging, but it doesn't show up in the logs (at the debug level)
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.
@TomSweeneyRedHat : apparently this (init
) happens before logging has been set up
That is, we are still running with the default log level of InfoLevel (hiding the debug logging)
Friendly ping. |
Not sure if there’s anything more that can be done here, or if we should continue to rename the binary... afaik it is called "podman" on Mac and Win, so maybe we need to put it in a weird PATH on Linux ? |
We are currently missing newer i.e. as we saw else where, the Linux download is missing and the others are stuck at 1.6.x still |
A friendly reminder that this PR had no activity for 30 days. |
@afbjorklund Still working on this? |
@rhatdan : No, I am not sure what kind of logging that @TomSweeneyRedHat wanted ? If we output something, that will break users (the same way that the docker wrapper does) I think we just went with everybody using https://minikube.sigs.k8s.io/docs/tasks/podman_service/
|
I am going through open pull requests at the moment. Apologies for not getting this in earlier! Much has changed since this PR was opened and while it doesn't apply as is anymore, we can still use the idea. The new remote client is talking to Podman's new rest API, so we can ignore varlink. But auto-detecting |
We have already adopted to the existing policy of |
I will close this PR, and you can make a new one for the new cilent (for the same original issue). We still have some issues with the documentation being "blank" and the > 1.6 binaries missing ? So it seems to be a lot more than just the name of the program that will need some rework here... Can continue the "podman-remote" discussion in the #4390 issue instead of here, if needed. |
If any of PODMAN_HOST or PODMAN_VARLINK_BRIDGE are set,
then execute "podman-remote" rather than this "podman".
Example:
Closes #4390