Skip to content
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

Whitespace in fully path to playbook causes rsync to fail #278

Conversation

asnaedae
Copy link

@asnaedae asnaedae commented Oct 8, 2021

Any whitespace in path to playbook directory causes rsync to incorrectly chdir fail to correctly run.

SUMMARY

Any whitespace in path to playbook directory causes rsync to incorrectly chdir fail to correctly run.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

ansible.posix.synchronize

ADDITIONAL INFORMATION

Example :

cd "/home/a/ansible plays"

task:
  - synchronize:
      src: a
      dest: b

Results in the following error being thrown

fatal: [remote-host]: FAILED! => {"changed": false, "cmd": "/usr/bin/rsync --delay-updates -F --compress --archive --rsh='/usr/bin/ssh -S none -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null' --rsync-path='sudo -u root rsync' --out-format='<<CHANGED>>%i %n%L' /home/a/ansible plays/deployments// remote-user@remote-host:/b/", "msg": "rsync: [sender] link_stat \"/home/a/ansible\" failed: No such file or directory (2)\nrsync: [sender] change_dir \"/home/a/ansible plays/plays/a/\" failed: No such file or directory (2)\nrsync error: some files/attrs were not transferred (see previous errors) (code 23) at main.c(1330) [sender=3.2.3]\n", "rc": 23}

@saito-hideki
Copy link
Collaborator

@asnaedae Hi! thank you for the PR!
Looking at the output of CI tests, it has failed with the following error:

Your pull-request is missing a changelog fragment, please add one. It should explain to end users the reason for your change.

In this case, you need to create a changelog fragments file under changelogs/fragments directory according to the following document:

Therefore, is it possible to create and add a fragments file and push those changes again?

@asnaedae asnaedae force-pushed the hotfix/escape_whitespace_in_paths branch from 8728c16 to 9374995 Compare October 21, 2021 09:41
Copy link
Collaborator

@saito-hideki saito-hideki left a comment

Choose a reason for hiding this comment

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

Hi! thanks for the PR!
I have left one change request regarding dest path handling. Also if it is possible, can you add some integration tests for source and dest handling with whitespaces in the full path?

…uses rsync to incorrectly chdir to current source dir,

Example :

cd "/home/a/ansible plays"

task:
  - synchronize:
      src: a
      dest: b

Results in the following error being thrown

fatal: [remote-host]: FAILED! => {"changed": false, "cmd": "/usr/bin/rsync --delay-updates -F --compress --archive --rsh='/usr/bin/ssh -S none -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null' --rsync-path='sudo -u root rsync' --out-format='<<CHANGED>>%i %n%L' /home/a/ansible plays/deployments// remote-user@remote-host:/data/", "msg": "rsync: [sender] link_stat \"/home/a/ansible\" failed: No such file or directory (2)\nrsync: [sender] change_dir \"/home/a/ansible plays/plays/a/\" failed: No such file or directory (2)\nrsync error: some files/attrs were not transferred (see previous errors) (code 23) at main.c(1330) [sender=3.2.3]\n", "rc": 23}
@asnaedae asnaedae force-pushed the hotfix/escape_whitespace_in_paths branch from 9374995 to d1be551 Compare October 25, 2021 18:44
@asnaedae
Copy link
Author

Thanks for the feedback @saito-hideki , I should have added tests and read the background docs before hitting the code, have added a test to cover both situations, and correctly asserts. Do let me know if there's any further changes to be made

thanks!

Copy link
Collaborator

@saito-hideki saito-hideki left a comment

Choose a reason for hiding this comment

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

Looks good to me! thanks :)

@saito-hideki
Copy link
Collaborator

@Akasurde if it is possible, can you review this pull request? As far as I can see, this PR looks reasonable for me.

@saito-hideki
Copy link
Collaborator

@asnaedae @Akasurde thanks a lot. I'll move forward with this PR.

@saito-hideki saito-hideki added the gate Gate PR in Zuul CI (Obsolete: Please set "mergeit" instead of "gate") label Nov 1, 2021
Copy link
Contributor

@ansible-zuul ansible-zuul bot left a comment

Choose a reason for hiding this comment

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

LGTM!

@saito-hideki
Copy link
Collaborator

Closing and re-opening for CI trigger

@saito-hideki saito-hideki reopened this Nov 1, 2021
@ansible-zuul ansible-zuul bot merged commit e366b90 into ansible-collections:main Nov 3, 2021
@asnaedae asnaedae deleted the hotfix/escape_whitespace_in_paths branch November 3, 2021 09:03
@Glandos
Copy link

Glandos commented Jul 9, 2024

I know this is old but… I only updated my playbook now (bye bye CentOS), and it breaks my synchronize usage, with multiple sources. Now, I have to loop over, but there is N output instead of one, it's much harder to aggregate.

I'll see if I have a workaround…

EDIT: I see this is addressed by #519

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gate Gate PR in Zuul CI (Obsolete: Please set "mergeit" instead of "gate")
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants