Skip to content

synchronize: Allow user-defined --out-format #428

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

growf
Copy link

@growf growf commented Mar 17, 2023

SUMMARY

If the user specifes --out-format in the synchronize module's rsync_opts return the msg and stdout_lines fields in the requested format. When no --out-format is specified the default format for --itemize-changes (%i %n%L) is used.

When the module is called with diff: true the returned diff will remain in the default format for --itemize-changes.

This is a change from the current behaviour that always forces an output format of %i %n%L regardless of user-specified values for --out-format. This current behaviour was hard-coded in order to facilitate correctly setting the changes return value and having a fixed format for diff: true. I've endeavored to maintain the current behaviour for those purposes whilst additionally allowing user-defined formats.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

synchronize

ADDITIONAL INFORMATION

The generated --out-format attempts to combine:

  • the user-requested --out-format
  • a fixed marker to identify formatted output lines, DIFF
  • the %i itemize-changes field
  • the %n%L filename and link information fields (if we are returning a diff or if no format was specified)

seperated by a delimiter of //. No delimiter is completely safe for this purpose but I felt this one was sufficiently unlikely to appear the final three fields.

I didn't attempt to use field widths for parsing given I've seen rsync versions with different widths for the %i field. In fact this unreliability of the %i field (added to the fact that it can contain internal spaces making the default pattern, %i %n%L, also tricky to split on a delimiter) is the motivation for this change. I wanted to pass an --out-format of {%-16i} %n to synchronize to make programmatic parsing of the returned output easier but discovered that the module couldn't support it.

I've avoided the use of some more modern Python syntax (str.removeprefix(), Match.__getitem__()) because I'm developing against Python 2.7.18 and I imagine that I'm not alone in having to support old versions.

If the user specifes --out-format in rsync_opts return msg and stdout_lines in that format otherwise use the default for --itemize-changes, '%i %n%L'; the output for diff will always be in the default --itemize-changes format.

The use of '//' as a delimiter to split the format fields isn't perfect but it's the least-likely to clash with an actual filepath that I could think of.
@growf growf changed the title Allow user-defined --out-format synchronize: Allow user-defined --out-format Mar 17, 2023
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/c4d77bd33dbe47e7be1e6a0e8c5d4bcf

✔️ ansible-changelog-fragment SUCCESS in 12s
✔️ ansible-test-sanity-docker-devel SUCCESS in 10m 38s (non-voting)
✔️ ansible-test-sanity-docker-milestone SUCCESS in 10m 15s (non-voting)
✔️ ansible-test-sanity-docker-stable-2.9 SUCCESS in 12m 24s
✔️ ansible-test-sanity-docker-stable-2.10 SUCCESS in 11m 18s
✔️ ansible-test-sanity-docker-stable-2.11 SUCCESS in 12m 25s
✔️ ansible-test-sanity-docker-stable-2.12 SUCCESS in 10m 16s
✔️ ansible-test-sanity-docker-stable-2.13 SUCCESS in 9m 17s
✔️ ansible-test-units-posix-python39 SUCCESS in 6m 45s
✔️ ansible-test-units-posix-python310 SUCCESS in 7m 10s
✔️ ansible-galaxy-importer SUCCESS in 4m 12s
✔️ build-ansible-collection SUCCESS in 6m 32s

@maxamillion
Copy link
Collaborator

@growf this looks great, thank you for the contribution! Can you please add some test cases to validate this new functionality? Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants