Update synchronize module plugin to work on OS X #12429

Merged
merged 3 commits into from Oct 9, 2015

Conversation

Projects
None yet
4 participants
@petrmifek
Contributor

petrmifek commented Sep 17, 2015

As discussed at the commit @6c107258fad96fca53bea5a74960f54ac0d7b45e Those changes broke synchronize module functionality on OS X, where the built in rsync is 2.6.9 and has some issues parsing the user@[hostname-or-ipv4-or-ipv6] format in destination.

Main issue is that it completely breaks the synchronize functionality even for IPv4 on current OS X.

The hack below seems to be working on both OS X and Linux (with newer rsync) and with IPv4/6 addresses and A and AAAA DNS names.

The rsync, especially on OS X, is quite picky about the format of SRC and DEST, so some independent testing would be good. Still, this allows for using the synchronize module again on a stock OS X without having to reinstall rsync on all the servers and clients.

Thanks for consideration!

Change synchronize module plugin to be backwards compatible with RSyn…
…c 2.6.9 with regard to handling IPv6 addresses.

petrmifek referenced this pull request Sep 17, 2015

Fix rsync connections to IPv6 addresses
Similar to #11816 we can unconditionally
wrap the host address in square brackets. This is required by rsync for IPv6
addresses.
@jaingaurav

This comment has been minimized.

Show comment
Hide comment
@jaingaurav

jaingaurav Sep 18, 2015

Contributor

An unfortunate ugly hack. Sorry I didn't do much testing on OS X, but I was able to confirm that this seems to be a reasonable fix.

Contributor

jaingaurav commented Sep 18, 2015

An unfortunate ugly hack. Sorry I didn't do much testing on OS X, but I was able to confirm that this seems to be a reasonable fix.

@abadger

This comment has been minimized.

Show comment
Hide comment
@abadger

abadger Oct 9, 2015

Member

@petrmifek If you'll move _needs_ipv6_brackets() up so it's not a nested function, this looks good to merge.

Member

abadger commented Oct 9, 2015

@petrmifek If you'll move _needs_ipv6_brackets() up so it's not a nested function, this looks good to merge.

@petrmifek

This comment has been minimized.

Show comment
Hide comment
@petrmifek

petrmifek Oct 9, 2015

Contributor

Thanks. Have a look now please.

Contributor

petrmifek commented Oct 9, 2015

Thanks. Have a look now please.

abadger added a commit that referenced this pull request Oct 9, 2015

Merge pull request #12429 from edmstudio/devel
Update synchronize module plugin to work on OS X

@abadger abadger merged commit 45a161b into ansible:devel Oct 9, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abadger

This comment has been minimized.

Show comment
Hide comment
@abadger

abadger Oct 9, 2015

Member

Thanks @petrmifek for the code and @jaingaurav for testing the change!

Merged

Hi!

This has been merged in, and will also be included in the next major release.

If you or anyone else has any further questions, please let us know by stopping by one of the two mailing lists, as appropriate:

Because this project is very active, we're unlikely to see comments made on closed tickets, but the mailing list is a great way to ask questions, or post if you don't think this particular
issue is resolved.

Thank you!

Member

abadger commented Oct 9, 2015

Thanks @petrmifek for the code and @jaingaurav for testing the change!

Merged

Hi!

This has been merged in, and will also be included in the next major release.

If you or anyone else has any further questions, please let us know by stopping by one of the two mailing lists, as appropriate:

Because this project is very active, we're unlikely to see comments made on closed tickets, but the mailing list is a great way to ask questions, or post if you don't think this particular
issue is resolved.

Thank you!

@ansibot ansibot added bug and removed bugfix_pull_request labels Mar 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment