-
Notifications
You must be signed in to change notification settings - Fork 149
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
Synchronize: suppress synchronize verbose output #220
base: main
Are you sure you want to change the base?
Synchronize: suppress synchronize verbose output #220
Conversation
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.
Thanks for the PR @mandar242!
Could you also add a changelog fragment: https://docs.ansible.com/ansible/latest/community/development_process.html#community-changelogs? |
Added. |
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.
Small nit-picks. Could you please add tests for this change? Thanks.
8cdd122
to
39930ca
Compare
cc @quidame @saito-hideki Could you please review this? Thanks in advance. |
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.
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 :)
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 @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. |
as reported by @gravesm , adding
The |
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.
^ The |
note that the 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.
@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__': |
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.
@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.
ignore_errors: true | ||
- assert: | ||
that: | ||
- '''files/directories have been synchronized'' not in sync_result.msg' |
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.
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
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
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. |
Ping? @Akasurde ? |
@phemmer sorry, but I am not actively looking into cc @saito-hideki @maxamillion Could you please take care of this PR? Thanks in advance. |
a072f0b
to
92dc532
Compare
Hello??? |
@gravesm Is this project maintained? |
Yes it is |
plugins/modules/synchronize.py
Outdated
- This option specifies quiet option which on true suppresses the output. | ||
type: bool | ||
default: no | ||
version_added: '1.3.0' |
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.
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 😄
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.
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..)
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.
added to 1.6.0
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'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.
It seems that the rebase 10 months ago reintroduced passing |
@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
92dc532
to
892c045
Compare
@phemmer the PR has been rebased successfully. waiting for reviews |
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 3m 52s |
@@ -601,6 +614,9 @@ def main(): | |||
|
|||
cmd.append(shlex_quote(source)) | |||
cmd.append(shlex_quote(dest)) | |||
if quiet: | |||
cmd.append('--quiet') | |||
|
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.
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.
SUMMARY
Adding rsync 'quiet' option to synchronize module.
Allows to suppress synchronize verbose output.
Resolves #171
ISSUE TYPE
COMPONENT NAME
Synchronize