Skip to content
This repository has been archived by the owner on Jan 8, 2023. It is now read-only.

Allow podman executable override with env #53

Merged
merged 21 commits into from
Aug 20, 2021

Conversation

NikolineLykPedersen
Copy link
Collaborator

@NikolineLykPedersen NikolineLykPedersen commented Aug 10, 2021

The idea behind this pull request is to add the posibility of changing the podman executable from the default, podman, to something else through an environment variable, MOLECULE_PODMAN_EXECUTABLE. This could for instance be podman-remote instead of podman. In that case the environment variable could be exported as:

$ export MOLECULE_PODMAN_EXECUTABLE=podman-remote

The variable has been named MOLECULE_PODMAN_EXECUTABLE to match the similar variable in ansible, namely ANSIBLE_PODMAN_EXECUTABLE

Changes have been made both in driver.py, create.yml, and destroy.yml, and have been tested with MOLECULE_PODMAN_EXECUTABLE=podman-remote

We needed this functionality at my work, and I thought others might be able to use it as well :)

Add possibility of setting podman executable through environment variable
Add podman_exec variable to enable using other podman executables, by setting environment variable MOLECULE_PODMAN_EXECUTABLE
Add podman_exec variable to enable using other podman executables, by setting environment variable MOLECULE_PODMAN_EXECUTABLE
@sshnaidm
Copy link
Collaborator

LGTM, can you please add it to documentation? (Do we have documentation? 👀 ) Or at least to write an example of usage in comments.

Update with short explanation on how to change podman executable
Make bold instead of subtitle
Add comment to explain how to change podman executable
@NikolineLykPedersen
Copy link
Collaborator Author

LGTM, can you please add it to documentation? (Do we have documentation? 👀 ) Or at least to write an example of usage in comments.

Of course :) I have attempted to add some explanation, both as a comment oin the code and as a couple of lines in the README.rst :)

@sshnaidm
Copy link
Collaborator

Great, thanks!

Divide comment into two lines
Divide comment into three lines
@ssbarnea
Copy link
Member

I am also using podman-remote on MacOS, but on that platform podman itself is podman-remote so no need to change anything.

On the other hand podman itself has the --remote argument so changing the executable does not make much sense to me.

Ideally podman should add --remote to the cli when CONTAINER_HOST variable is defined. If it does not do that yet, I would support a feature in molecule driver that does this.

Still, I do not support a feature that is not portable and podman-remote does not exist on macos, there is only podman.

Something like `if not-macos and CONTAINER_HOST in os.environ: add '--remote' to podman cli makes more sense to me.

Before doing anything about this lets consult with podman cores, as they may give us some hints on the future plans, so we can avoid a hack that could easily break.

src/molecule_podman/driver.py Outdated Show resolved Hide resolved
@sshnaidm
Copy link
Collaborator

I am also using podman-remote on MacOS, but on that platform podman itself is podman-remote so no need to change anything.

On the other hand podman itself has the --remote argument so changing the executable does not make much sense to me.

Ideally podman should add --remote to the cli when CONTAINER_HOST variable is defined. If it does not do that yet, I would support a feature in molecule driver that does this.

Still, I do not support a feature that is not portable and podman-remote does not exist on macos, there is only podman.

Something like `if not-macos and CONTAINER_HOST in os.environ: add '--remote' to podman cli makes more sense to me.

No need depend on CONTAINER_HOST var, podman-remote can work without it with additional external config, it's completely not required.
If MacOS doesn't support something, we can add a comment and add an issue, it's not a reason to hold all 99.999% of users that don't use podman-remote on MacOS and it's not an issue of molecule-podman project.

Change formatting of ansible_connection_options
add comma in ansible_connection_options
Copy link
Member

@ssbarnea ssbarnea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please wait until we get feedback from podman team on containers/podman#11196 -- based on that we can decide which way to do.

To be clear I am extremely interested about ensuring podman remoting support works fine with our driver, is critical.

@ssbarnea ssbarnea added the feature This issue/PR relates to a feature request. label Aug 11, 2021
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change may be valuable in some cases but I second Sorin's feedback — we way want to wait for the upstream to make a related decision first.

Meanwhile, I think that there's a workaround — add a custom podman script to some directory that you manage in front of the $PATH, and in that wrapper script, you could choose to run anything you like (for now).

@sshnaidm
Copy link
Collaborator

@NikolineLykPedersen you can try to export Ansible environment variable as export ANSIBLE_PODMAN_EXECUTABLE=podman-remote when running molecule.
I don't agree that this patch should solve all podman remote problems, whether they exist or not, and don't see any blockers to merge this because it just pass through the remote option that exists in ansible podman collection. All the rest is completely out of scope and not related.

@ssbarnea
Copy link
Member

I think we do have a very good reason for creating a test for remote podman usage inside both the collection and this driver. That would help us avoid regressions in this area. Still, that would not be the subject of this PR.

@ssbarnea
Copy link
Member

@sshnaidm Was ANSIBLE_PODMAN_EXECUTABLE a typo or an idea? I was not able to find anything about it on google but I kinda like avoiding adding a molecule specific environment variable. Something like ANSIBLE_PODMAN_EXECUTABLE or even PODMAN_EXECUTABLE could be easier to reuse by multiple tools for the same goal.

On the other hand, I know that podman environment variables do start with CONTAINER_, so maybe even using that prefix could make sense.

@sshnaidm
Copy link
Collaborator

ANSIBLE_PODMAN_EXECUTABLE exists in Ansible Podman connection plugin for years: https://github.com/containers/ansible-podman-collections/blob/d1698121a9585f63dd7eb7ab5ba1eeb02c60f995/plugins/connection/podman.py#L63-L70
Together with ANSIBLE_PODMAN_EXTRA_ARGS for podman executable. It's there from 2.9: ansible/ansible@e3c7e35

NikolineLykPedersen and others added 4 commits August 13, 2021 08:49
Change from bold to title

Co-authored-by: Sviatoslav Sydorenko <[email protected]>
Co-authored-by: Sviatoslav Sydorenko <[email protected]>
Co-authored-by: Sviatoslav Sydorenko <[email protected]>
Move definition of podman_exec out of the module-level + add step to find full path for podman_exec and give error, if it can't be found
@NikolineLykPedersen
Copy link
Collaborator Author

@NikolineLykPedersen you can try to export Ansible environment variable as export ANSIBLE_PODMAN_EXECUTABLE=podman-remote when running molecule.

I've tried with ANSIBLE_PODMAN_EXECUTABLE=podman-remote, but it doesn't affect how containers are created or logged into by molecule. But thanks for the suggestion :)

I might look at the workaround suggested above. Thanks for all the feedback :)

Add task to determine path to podman executable + use path to poodman executable in podman login and build commands
Also use path to executable in Create molecule instance + correct way path was stored as variable
@ssbarnea ssbarnea changed the title Add possibility of changing podman executable through environment variable Allow podman executable override with env Aug 19, 2021
Copy link
Member

@ssbarnea ssbarnea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still find this a hack that we may need to remove in the future but on the other hand a workaround today may prove more useful than a perfect solution in 2 years.

@ssbarnea ssbarnea enabled auto-merge (squash) August 19, 2021 14:02
auto-merge was automatically disabled August 19, 2021 14:12

Head branch was pushed to by a user without write access

Add importstatements
@ssbarnea
Copy link
Member

@NikolineLykPedersen Check https://github.com/ansible-community/molecule-podman/invitations that should allow you to test changes without manual approval.

@ssbarnea
Copy link
Member

@NikolineLykPedersen please fix the linting problems. If you do it quick we may include that in 1.0.0 release.

You can use changed_when: true/false to avoid these errors.

Fix linting errors
Remove trailing whitespaces
fix linting issues
Fix more linting issues
@NikolineLykPedersen
Copy link
Collaborator Author

All the linting problems should be fixed now :)

@ssbarnea ssbarnea merged commit e120aa1 into ansible-community:main Aug 20, 2021
gs-kamnas pushed a commit to gs-kamnas/molecule-podman that referenced this pull request Jan 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature This issue/PR relates to a feature request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants