-
Notifications
You must be signed in to change notification settings - Fork 154
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
Complete adding podman connection to synchronize #230
Conversation
@tadeboro please take a look if it's OK. |
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 am not an expert at this module, but I would expect that the podman needs a similar special treatment just like the docker and buildah connection plugins have at
ansible.posix/plugins/action/synchronize.py
Lines 416 to 424 in 1ebacfb
if self._remote_transport in ['docker', 'community.general.docker', 'community.docker.docker']: | |
if become and self._play_context.become_user: | |
_tmp_args['rsync_opts'].append("--rsh=%s exec -u %s -i" % (self._docker_cmd, self._play_context.become_user)) | |
elif user is not None: | |
_tmp_args['rsync_opts'].append("--rsh=%s exec -u %s -i" % (self._docker_cmd, user)) | |
else: | |
_tmp_args['rsync_opts'].append("--rsh=%s exec -i" % self._docker_cmd) | |
elif self._remote_transport in ['buildah', 'containers.podman.buildah']: | |
_tmp_args['rsync_opts'].append("--rsh=buildah run --") |
I do not have access to my work machine with the podman installed right now, which means that I cannot confirm if any special treatment is actually needed here. I will try to test this out in about 12 hours.
Yeah, probably it requires more work, moving to draft for now |
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.
Also, add a changelog entry.
Codecov Report
@@ Coverage Diff @@
## main #230 +/- ##
=======================================
Coverage 25.65% 25.65%
=======================================
Files 41 41
Lines 3918 3918
Branches 770 770
=======================================
Hits 1005 1005
Misses 2716 2716
Partials 197 197 Continue to review full report at Codecov.
|
May I suggest disabling codecov comments? It requires only one line https://github.com/ansible-community/ansible-compat/blob/main/codecov.yml#L1 and it does not disable codecov a run and ability to browse the full reports if you press on the job result. Still, no more spam in PR conversations. |
I just tested the code from this PR and things still failed. I am not sure what is the main reason, but I just wanted to show what I see. Playbook:
Inventory:
Output:
|
@tadeboro how did you prepare your container?
and running your playbook pass. |
recheck |
I used https://hub.docker.com/_/nginx/ as a base, used
|
@tadeboro I ran your config and still works fine, please take a look maybe some differences:
|
/rebuild_failed |
I have no further ideas about where the difference would come from. The host system I use is a fresh Fedora 34 system and as far as I can tell, I made no changes to the default podman configuration. |
As I see the error is
|
This seems to be an issue with my setup, so feel free to merge this PR. I can try to determine what is going awry without making a mess out of this PR. Thanks for the help anyhow, @sshnaidm. |
@tadeboro thanks for test anyway, probably worth someone else to check the same. So we can be sure it's not something with my setup 😄 |
I managed to find what is causing errors on my machine. The culprit is the lack of proper quoting around some rsync arguments. The command that fails is
When I patched the action plugin a bit to produce the following command, things started working (the difference is in the
But this has nothing to do with this PR, so I think current changes are ready to ship once AZP stops messing around. I will file a bug for my issue so that it does not get lost. |
/rebuild_failed |
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.
LGTM
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.
LGTM!
SUMMARY
ISSUE TYPE
COMPONENT NAME
synchronize
ADDITIONAL INFORMATION
Continue of #229
For fixing ansible-community/molecule-plugins#81