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

fix: simplify podman executable path lookup #142

Merged
merged 6 commits into from
Dec 6, 2022

Conversation

gardar
Copy link
Contributor

@gardar gardar commented Oct 19, 2022

Avoids unnecessary command tasks and removes which as a dependency that is not always present in minimal environments. (It's not available in https://github.com/ansible/creator-ee for example).

@apatard
Copy link
Member

apatard commented Oct 20, 2022

I don't know the playbook but I'm wondering why you're not changing things to use containers.podman collection ?

@gardar
Copy link
Contributor Author

gardar commented Oct 20, 2022

@apatard I'm not a maintainer and I'm just submitting fixes/improvements for issues that I'm running into while using the driver, but I've actually wondered about that myself, why this driver is using the shell/command modules instead of the podman modules.
I can't find any issues/discussions on the subject so I'm guessing that at the time the driver was made the podman modules didn't exist or were too limited.

I opened a issue on the subject where we can discuss this further. ansible-community/molecule-plugins#78

@ssbarnea
Copy link
Member

I can answer this one. Podman collection was not really actively maintained and that gave more flexibility. Things change, I am not against using it, but be sure it is maintained.

@zhan9san zhan9san added the bug This issue/PR relates to a bug. label Oct 21, 2022
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.

Use of lookup is not acceptable as it runs on control machine! Good that I remembered that before merging it.

@ssbarnea
Copy link
Member

Basically the real fix for this bug is a single line change that replaces which with command -v, nothing else.

@gardar
Copy link
Contributor Author

gardar commented Oct 24, 2022

Use of lookup is not acceptable as it runs on control machine! Good that I remembered that before merging it.

But isn't this step of driver always running on the controller anyways?

@ssbarnea
Copy link
Member

Use of lookup is not acceptable as it runs on control machine! Good that I remembered that before merging it.

But isn't this step of driver always running on the controller anyways?

Happens to be on controlled does not mean it will always be that. Think about users using remote container machines. I would prefer to use command -v which is safe and portable.

@gardar
Copy link
Contributor Author

gardar commented Oct 25, 2022

Use of lookup is not acceptable as it runs on control machine! Good that I remembered that before merging it.

But isn't this step of driver always running on the controller anyways?

Happens to be on controlled does not mean it will always be that. Think about users using remote container machines. I would prefer to use command -v which is safe and portable.

Ok I'm fine with command -v, it should be built into most common shells.

I didn't realize that molecule could execute drivers that are located on a different host than molecule itself. But since that's the case I assume we should get rid of all the other lookups too?

@ssbarnea
Copy link
Member

Depends. Not all lookups are risky.

@zhan9san
Copy link
Member

Think about users using remote container machines.

IMHO, the container client will still run on the controller, does't it?

@gardar
Copy link
Contributor Author

gardar commented Oct 27, 2022

Depends. Not all lookups are risky.

I didn't think that the first found lookup would be considered risky, I thought the issue with using a lookup would be because the lookup simply wouldn't work.

IMHO, the container client will still run on the controller, does't it?

That was what I thought and then the lookups would behave exactly like any other task, since it's being ran against localhost anyways.
But since that is not the case, I'm curious as to how you can do it remotely as I haven't come across any documentation hinting as to how you can execute the driver remotely.

@gardar gardar requested a review from ssbarnea November 16, 2022 10:41
@gardar gardar requested review from apatard and ssbarnea and removed request for ssbarnea and apatard December 1, 2022 13:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug This issue/PR relates to a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants