Synchronize: suppress synchronize verbose output #220

mandar242 · 2021-07-12T16:36:53Z

SUMMARY

Adding rsync 'quiet' option to synchronize module.
Allows to suppress synchronize verbose output.

Resolves #171

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

Synchronize

gravesm

Thanks for the PR @mandar242!

plugins/modules/synchronize.py

gravesm · 2021-07-12T18:02:02Z

Could you also add a changelog fragment: https://docs.ansible.com/ansible/latest/community/development_process.html#community-changelogs?

mandar242 · 2021-07-12T19:05:17Z

Could you also add a changelog fragment: https://docs.ansible.com/ansible/latest/community/development_process.html#community-changelogs?

Added.

Akasurde

Small nit-picks. Could you please add tests for this change? Thanks.

changelogs/fragments/220_synchronize_add_quiet_option.yml

plugins/modules/synchronize.py

Akasurde · 2021-07-14T04:12:13Z

cc @quidame @saito-hideki Could you please review this? Thanks in advance.

saito-hideki

Hi! thank you for sending the PR!
Currently, we need to append --allow-disabled to perform the integration tests, but this PR and the integration tests worked fine in my testing environment.
BTW, a fix for the aliases file to enable the integration tests has been already included in another PR #213. So this PR looks reasonable to me even if it does not include the aliases file changing :)

gravesm · 2021-07-14T14:45:55Z

This change actually may not work as is. It's breaking the ability to determine changed status. Testing locally for me shows no change when there is, in fact, a change. I think I'm not sure what extraneous output the OP was referring to is. The only case I can find where -v is being used is when link_dest is set, and the note in the code seems to suggest this is required here. @phemmer can you clarify what output you are referring to and possibly provide an example task to reproduce?

@mandar242 pending further information about what's being requested in this feature, we should add a few more tests to this. At least one to check that a task's result has changed and then a following one running the same task to check that the result has not changed.

quidame · 2021-07-14T15:36:34Z

as reported by @gravesm , adding --quiet option just breaks check_mode. As said in rsync(1) manpage:

-q, --quiet                 suppress non-error messages

The check_mode and diff are based on --out-format option (with a marker in), which is suppressed by the use of --quiet.

saito-hideki

Sorry guys, I missed the point pointed out by @gravesm and @quidame. I'll revert my approval and wait for updates

phemmer · 2021-07-22T15:30:45Z

@phemmer can you clarify what output you are referring to and possibly provide an example task to reproduce?

$ mkdir /tmp/a

$ touch /tmp/a/{1..100}

$ ansible localhost -m synchronize -a 'src=/tmp/a/ dest=/tmp/b/'
[WARNING]: No inventory was parsed, only implicit localhost is available
localhost | CHANGED => {
    "changed": true,
    "cmd": "/usr/bin/rsync --delay-updates -F --compress --archive --out-format=<<CHANGED>>%i %n%L /tmp/a/ /tmp/b/",
    "msg": "cd+++++++++ ./\n>f+++++++++ 1\n>f+++++++++ 10\n>f+++++++++ 100\n>f+++++++++ 11\n>f+++++++++ 12\n>f+++++++++ 13\n>f+++++++++ 14\n>f+++++++++ 15\n>f+++++++++ 16\n>f+++++++++ 17\n>f+++++++++ 18\n>f+++++++++ 19\n>f+++++++++ 2\n>f+++++++++ 20\n>f+++++++++ 21\n>f+++++++++ 22\n>f+++++++++ 23\n>f+++++++++ 24\n>f+++++++++ 25\n>f+++++++++ 26\n>f+++++++++ 27\n>f+++++++++ 28\n>f+++++++++ 29\n>f+++++++++ 3\n>f+++++++++ 30\n>f+++++++++ 31\n>f+++++++++ 32\n>f+++++++++ 33\n>f+++++++++ 34\n>f+++++++++ 35\n>f+++++++++ 36\n>f+++++++++ 37\n>f+++++++++ 38\n>f+++++++++ 39\n>f+++++++++ 4\n>f+++++++++ 40\n>f+++++++++ 41\n>f+++++++++ 42\n>f+++++++++ 43\n>f+++++++++ 44\n>f+++++++++ 45\n>f+++++++++ 46\n>f+++++++++ 47\n>f+++++++++ 48\n>f+++++++++ 49\n>f+++++++++ 5\n>f+++++++++ 50\n>f+++++++++ 51\n>f+++++++++ 52\n>f+++++++++ 53\n>f+++++++++ 54\n>f+++++++++ 55\n>f+++++++++ 56\n>f+++++++++ 57\n>f+++++++++ 58\n>f+++++++++ 59\n>f+++++++++ 6\n>f+++++++++ 60\n>f+++++++++ 61\n>f+++++++++ 62\n>f+++++++++ 63\n>f+++++++++ 64\n>f+++++++++ 65\n>f+++++++++ 66\n>f+++++++++ 67\n>f+++++++++ 68\n>f+++++++++ 69\n>f+++++++++ 7\n>f+++++++++ 70\n>f+++++++++ 71\n>f+++++++++ 72\n>f+++++++++ 73\n>f+++++++++ 74\n>f+++++++++ 75\n>f+++++++++ 76\n>f+++++++++ 77\n>f+++++++++ 78\n>f+++++++++ 79\n>f+++++++++ 8\n>f+++++++++ 80\n>f+++++++++ 81\n>f+++++++++ 82\n>f+++++++++ 83\n>f+++++++++ 84\n>f+++++++++ 85\n>f+++++++++ 86\n>f+++++++++ 87\n>f+++++++++ 88\n>f+++++++++ 89\n>f+++++++++ 9\n>f+++++++++ 90\n>f+++++++++ 91\n>f+++++++++ 92\n>f+++++++++ 93\n>f+++++++++ 94\n>f+++++++++ 95\n>f+++++++++ 96\n>f+++++++++ 97\n>f+++++++++ 98\n>f+++++++++ 99\n",
    "rc": 0,
    "stdout_lines": [
        "cd+++++++++ ./",
        ">f+++++++++ 1",
        ">f+++++++++ 10",
        ">f+++++++++ 100",
        ">f+++++++++ 11",
        ">f+++++++++ 12",
        ">f+++++++++ 13",
...
        ">f+++++++++ 96",
        ">f+++++++++ 97",
        ">f+++++++++ 98",
        ">f+++++++++ 99"
    ]
}

^ The msg is the problem, as that's what ansible-playbook is going to display.

quidame · 2021-07-22T17:30:28Z

note that the --out-format option is given a marker (<<CHANGED>>), which is removed from the output before sending this output in the results (msg, stdout_lines). At this step (the module post-processing rsync results, and being aware of possible changes), I think it is doable to just keep rsync's stderr and suppress rsync's stdout if a user requests that with a quiet=true new module option :)

Maybe keep transfer metadata (number of files, duration...) anyway, even when hidding all the details, could be a good point.

Reviewing this PR again based on the comments from other maintainers. In the meantime, revert my review status from approved to pending.

quidame · 2021-07-24T00:12:06Z

@mandar242 , I suggest you try with something like the following, which shouldn't break anything

@@ -619,14 +627,20 @@ def main():
     out_lines = out_clean.split('\n')
     while '' in out_lines:
         out_lines.remove('')
+
+    result = dict(changed=changed, rc=rc, cmd=cmdstr)
+
+    if quiet:
+        result['msg'] = "OUTPUT IS HIDDEN DUE TO 'quiet=true'"
+        result['stdout_lines'] = []
+    else:
+        result['msg'] = out_clean
+        result['stdout_lines'] = out_lines
+
     if module._diff:
-        diff = {'prepared': out_clean}
-        return module.exit_json(changed=changed, msg=out_clean,
-                                rc=rc, cmd=cmdstr, stdout_lines=out_lines,
-                                diff=diff)
+        result['diff'] = {'prepared': out_clean}
 
-    return module.exit_json(changed=changed, msg=out_clean,
-                            rc=rc, cmd=cmdstr, stdout_lines=out_lines)
+    return module.exit_json(**result)
 
 
 if __name__ == '__main__':

plugins/modules/synchronize.py

quidame

@mandar242 , please take care to not break the existing interface. In f508845 you changed the stdout_lines result into std_out, please don't do that.

plugins/modules/synchronize.py

tests/integration/targets/synchronize/tasks/main.yml

quidame · 2021-07-28T23:37:20Z

tests/integration/targets/synchronize/tasks/main.yml

+ ignore_errors: true
+- assert:
+ that:
+ - '''files/directories have been synchronized'' not in sync_result.msg'


This doesn't really make sense for me: despite this assertion, we have no certainty that this string is not in the result msg when the synchronize module succeeds, because we have no certainty that the synchronize module has succeeded, from a logic point of view.

Unless one wants to trigger an error, for example to test error handling of the module and ensure it behaves correctly under stress or with bad parameter values - and in that case we also want the playbook to not fail if a given task fails as expected - please don't use ignore_errors=true. Never. This makes just the tests passing even if the module is broken, that is exactly what the tests are made to avoid. An example of consistent use of ignore_errors: https://github.com/ansible-collections/community.general/blob/main/tests/integration/targets/filesize/tasks/errors.yml

tests/integration/targets/synchronize/tasks/main.yml

plugins/modules/synchronize.py

tests/integration/targets/synchronize/tasks/main.yml

quidame

LGTM

phemmer · 2023-08-08T15:42:38Z

2 years later, what's needed for this to be merged? I see there's a change requested to add tests, but it looks like tests are present.

phemmer · 2023-08-18T15:58:44Z

Ping? @Akasurde ?

Akasurde · 2023-08-18T17:20:28Z

@phemmer sorry, but I am not actively looking into ansible.posix collection. @mandar242 Can you please rebase the branch and update version_added?

cc @saito-hideki @maxamillion Could you please take care of this PR? Thanks in advance.

phemmer · 2023-11-17T12:59:51Z

@saito-hideki @maxamillion

Hello???

phemmer · 2024-06-07T13:20:21Z

@gravesm
@saito-hideki
@quidame
@Akasurde
@jillr
@maxamillion
@mrtwnklr
@bachradsusi
@s-hertel
@mib1185
@cwchristerw
@felixfontein

Is this project maintained?

cwchristerw · 2024-06-07T17:10:46Z

@gravesm @saito-hideki @quidame @Akasurde @jillr @maxamillion @mrtwnklr @bachradsusi @s-hertel @mib1185 @cwchristerw @felixfontein

Is this project maintained?

Yes it is

cwchristerw · 2024-06-07T17:26:29Z

plugins/modules/synchronize.py

+ - This option specifies quiet option which on true suppresses the output.
+ type: bool
+ default: no
+ version_added: '1.3.0'


Next minor version is 1.6.x. I think new features to modules would need to be in next minor release. Ps. I'm new to this, so I'm not sure 😄

It's 1.6.0, not 1.6.x :) But yes, that will be the next minor version, and the next one that will get released. (The only question is when..)

added to 1.6.0

felixfontein

I'm not sure that this option makes sense, since --quiet means "suppress non-error messages". But the module needs the output to determine whether something changed.

So IMO this feature breaks the module and should not be merged.

plugins/modules/synchronize.py

felixfontein · 2024-06-07T19:45:33Z

It seems that the rebase 10 months ago reintroduced passing --quiet to rsync (the problem has been discussed and fixed before that rebase).

phemmer · 2024-06-07T20:36:33Z

@mandar242 since it has been years, if you no longer have the interest to maintain this PR, let me know and I can pick it up and open a new one.

mandar242 · 2024-06-07T20:53:07Z

@mandar242 since it has been years, if you no longer have the interest to maintain this PR, let me know and I can pick it up and open a new one.

@phemmer I am fine with either. But given the number of commits made over the time, I think it will be easier and quicker to open new PR with changes from this pr. Rebase could be complex.

… print error only

mandar242 · 2024-06-07T21:55:34Z

@mandar242 since it has been years, if you no longer have the interest to maintain this PR, let me know and I can pick it up and open a new one.

@phemmer I am fine with either. But given the number of commits made over the time, I think it will be easier and quicker to open new PR with changes from this pr. Rebase could be complex.

@phemmer the PR has been rebased successfully. waiting for reviews
cc @maxamillion @saito-hideki @felixfontein

softwarefactory-project-zuul · 2024-06-07T22:21:57Z

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

✔️ ansible-galaxy-importer SUCCESS in 3m 52s
✔️ build-ansible-collection SUCCESS in 12m 36s

phemmer · 2024-06-07T23:38:03Z

plugins/modules/synchronize.py

@@ -601,6 +614,9 @@ def main():

 cmd.append(shlex_quote(source))
 cmd.append(shlex_quote(dest))
+ if quiet:
+ cmd.append('--quiet')
+


This break the change detection as @felixfontein was mentioning here

Plus we probably want to keep the changes in stdout_lines. It's only msg that we want to suppress them from.

mandar242 changed the title ~~Issues/171/suppress synchronize verbose output~~ Synchronize: suppress synchronize verbose output Jul 12, 2021

gravesm requested changes Jul 12, 2021

View reviewed changes

plugins/modules/synchronize.py Outdated Show resolved Hide resolved

plugins/modules/synchronize.py Outdated Show resolved Hide resolved

gravesm mentioned this pull request Jul 12, 2021

Suppress syncronize verbose output #171

Open

Akasurde requested changes Jul 13, 2021

View reviewed changes

changelogs/fragments/220_synchronize_add_quiet_option.yml Outdated Show resolved Hide resolved

plugins/modules/synchronize.py Outdated Show resolved Hide resolved

plugins/modules/synchronize.py Outdated Show resolved Hide resolved

Akasurde force-pushed the issues/171/suppress-synchronize-verbose-output branch from 8cdd122 to 39930ca Compare July 14, 2021 04:10

saito-hideki previously approved these changes Jul 14, 2021

View reviewed changes

saito-hideki reviewed Jul 15, 2021

View reviewed changes

phemmer reviewed Jul 27, 2021

View reviewed changes

plugins/modules/synchronize.py Outdated Show resolved Hide resolved

plugins/modules/synchronize.py Outdated Show resolved Hide resolved

quidame suggested changes Jul 28, 2021

View reviewed changes

plugins/modules/synchronize.py Outdated Show resolved Hide resolved

tests/integration/targets/synchronize/tasks/main.yml Outdated Show resolved Hide resolved

tests/integration/targets/synchronize/tasks/main.yml Outdated Show resolved Hide resolved

quidame reviewed Jul 28, 2021

View reviewed changes

quidame reviewed Jul 29, 2021

View reviewed changes

plugins/modules/synchronize.py Outdated Show resolved Hide resolved

quidame reviewed Aug 5, 2021

View reviewed changes

tests/integration/targets/synchronize/tasks/main.yml Outdated Show resolved Hide resolved

tests/integration/targets/synchronize/tasks/main.yml Outdated Show resolved Hide resolved

tests/integration/targets/synchronize/tasks/main.yml Outdated Show resolved Hide resolved

quidame approved these changes Aug 6, 2021

View reviewed changes

mandar242 requested review from gravesm and Akasurde August 6, 2021 00:34

gravesm approved these changes Aug 6, 2021

View reviewed changes

saito-hideki approved these changes Aug 7, 2021

View reviewed changes

mandar242 force-pushed the issues/171/suppress-synchronize-verbose-output branch from a072f0b to 92dc532 Compare August 22, 2023 19:07

phemmer mentioned this pull request Nov 28, 2023

Can someone please look at PR #220 #513

Open

cwchristerw reviewed Jun 7, 2024

View reviewed changes

felixfontein suggested changes Jun 7, 2024

View reviewed changes

plugins/modules/synchronize.py Outdated Show resolved Hide resolved

plugins/modules/synchronize.py Outdated Show resolved Hide resolved

mandar242 and others added 10 commits June 7, 2024 14:40

Adding parameter to synchronize module to suppress verbose output and…

8bb6104

… print error only

Adding rsync parameter 'quiet' to synchronize

1a4c205

Added documentation and example for quiet option

fe7dd71

Fix: moving conditional to correct place

1e0654f

Added Changelog fragment

ee77487

Modifying based on feedback

6afd4cd

Adding tests, corrections

e72424a

Modifying output to be displayed based on feedback

b4fe18e

Modify based on feedback

793518b

rebase

892c045

mandar242 force-pushed the issues/171/suppress-synchronize-verbose-output branch from 92dc532 to 892c045 Compare June 7, 2024 21:46

mandar242 added 2 commits June 7, 2024 14:48

modified based on feedback

0a58f59

sanity fix in tests

48c2e93

mandar242 requested review from phemmer, cwchristerw and felixfontein June 7, 2024 21:55

mandar242 added 2 commits June 7, 2024 14:58

sanity fix in tests

9405976

sanity fix in tests

c401a5d

phemmer reviewed Jun 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Synchronize: suppress synchronize verbose output #220

Synchronize: suppress synchronize verbose output #220

mandar242 commented Jul 12, 2021

gravesm left a comment

gravesm commented Jul 12, 2021

mandar242 commented Jul 12, 2021

Akasurde left a comment

Akasurde commented Jul 14, 2021

saito-hideki left a comment

gravesm commented Jul 14, 2021

quidame commented Jul 14, 2021 •

edited

Loading

saito-hideki left a comment

phemmer commented Jul 22, 2021 •

edited

Loading

quidame commented Jul 22, 2021

quidame commented Jul 24, 2021

quidame left a comment

quidame Jul 28, 2021

quidame left a comment

phemmer commented Aug 8, 2023

phemmer commented Aug 18, 2023

Akasurde commented Aug 18, 2023

phemmer commented Nov 17, 2023

phemmer commented Jun 7, 2024

cwchristerw commented Jun 7, 2024

cwchristerw Jun 7, 2024 •

edited

Loading

felixfontein Jun 7, 2024

mandar242 Jun 7, 2024

felixfontein left a comment

felixfontein commented Jun 7, 2024

phemmer commented Jun 7, 2024

mandar242 commented Jun 7, 2024

mandar242 commented Jun 7, 2024

softwarefactory-project-zuul bot commented Jun 7, 2024

phemmer Jun 7, 2024

Synchronize: suppress synchronize verbose output #220

Are you sure you want to change the base?

Synchronize: suppress synchronize verbose output #220

Conversation

mandar242 commented Jul 12, 2021

SUMMARY

ISSUE TYPE

COMPONENT NAME

gravesm left a comment

Choose a reason for hiding this comment

gravesm commented Jul 12, 2021

mandar242 commented Jul 12, 2021

Akasurde left a comment

Choose a reason for hiding this comment

Akasurde commented Jul 14, 2021

saito-hideki left a comment

Choose a reason for hiding this comment

gravesm commented Jul 14, 2021

quidame commented Jul 14, 2021 • edited Loading

saito-hideki left a comment

Choose a reason for hiding this comment

phemmer commented Jul 22, 2021 • edited Loading

quidame commented Jul 22, 2021

quidame commented Jul 24, 2021

quidame left a comment

Choose a reason for hiding this comment

quidame Jul 28, 2021

Choose a reason for hiding this comment

quidame left a comment

Choose a reason for hiding this comment

phemmer commented Aug 8, 2023

phemmer commented Aug 18, 2023

Akasurde commented Aug 18, 2023

phemmer commented Nov 17, 2023

phemmer commented Jun 7, 2024

cwchristerw commented Jun 7, 2024

cwchristerw Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

felixfontein Jun 7, 2024

Choose a reason for hiding this comment

mandar242 Jun 7, 2024

Choose a reason for hiding this comment

felixfontein left a comment

Choose a reason for hiding this comment

felixfontein commented Jun 7, 2024

phemmer commented Jun 7, 2024

mandar242 commented Jun 7, 2024

mandar242 commented Jun 7, 2024

softwarefactory-project-zuul bot commented Jun 7, 2024

phemmer Jun 7, 2024

Choose a reason for hiding this comment

quidame commented Jul 14, 2021 •

edited

Loading

phemmer commented Jul 22, 2021 •

edited

Loading

cwchristerw Jun 7, 2024 •

edited

Loading