-
Notifications
You must be signed in to change notification settings - Fork 23.7k
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
vyos_config: print compare command result in diff mode #42494
Conversation
@@ -223,7 +223,7 @@ def run(module, result): | |||
result['changed'] = True | |||
|
|||
if module._diff: | |||
result['diff'] = diff | |||
result['diff'] = {'prepared': diff} |
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.
Is there a specific reason for moving generated diff under prepared key?
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.
@ganeshrn Thank you for comment.
Just for printing diff.
I think printing diff need (before
, after
) or prepared
keys.
ansible/lib/ansible/plugins/callback/__init__.py
Lines 151 to 219 in 9c5d40f
def _get_diff(self, difflist): | |
if not isinstance(difflist, list): | |
difflist = [difflist] | |
ret = [] | |
for diff in difflist: | |
try: | |
with warnings.catch_warnings(): | |
warnings.simplefilter('ignore') | |
if 'dst_binary' in diff: | |
ret.append("diff skipped: destination file appears to be binary\n") | |
if 'src_binary' in diff: | |
ret.append("diff skipped: source file appears to be binary\n") | |
if 'dst_larger' in diff: | |
ret.append("diff skipped: destination file size is greater than %d\n" % diff['dst_larger']) | |
if 'src_larger' in diff: | |
ret.append("diff skipped: source file size is greater than %d\n" % diff['src_larger']) | |
if 'before' in diff and 'after' in diff: | |
# format complex structures into 'files' | |
for x in ['before', 'after']: | |
if isinstance(diff[x], MutableMapping): | |
diff[x] = json.dumps(diff[x], sort_keys=True, indent=4, separators=(',', ': ')) + '\n' | |
if 'before_header' in diff: | |
before_header = "before: %s" % diff['before_header'] | |
else: | |
before_header = 'before' | |
if 'after_header' in diff: | |
after_header = "after: %s" % diff['after_header'] | |
else: | |
after_header = 'after' | |
before_lines = to_text(diff['before']).splitlines(True) | |
after_lines = to_text(diff['after']).splitlines(True) | |
if before_lines and not before_lines[-1].endswith('\n'): | |
before_lines[-1] += '\n\\ No newline at end of file\n' | |
if after_lines and not after_lines[-1].endswith('\n'): | |
after_lines[-1] += '\n\\ No newline at end of file\n' | |
differ = difflib.unified_diff(before_lines, | |
after_lines, | |
fromfile=before_header, | |
tofile=after_header, | |
fromfiledate='', | |
tofiledate='', | |
n=C.DIFF_CONTEXT) | |
difflines = list(differ) | |
if len(difflines) >= 3 and sys.version_info[:2] == (2, 6): | |
# difflib in Python 2.6 adds trailing spaces after | |
# filenames in the -- before/++ after headers. | |
difflines[0] = difflines[0].replace(' \n', '\n') | |
difflines[1] = difflines[1].replace(' \n', '\n') | |
# it also treats empty files differently | |
difflines[2] = difflines[2].replace('-1,0', '-0,0').replace('+1,0', '+0,0') | |
has_diff = False | |
for line in difflines: | |
has_diff = True | |
if line.startswith('+'): | |
line = stringc(line, C.COLOR_DIFF_ADD) | |
elif line.startswith('-'): | |
line = stringc(line, C.COLOR_DIFF_REMOVE) | |
elif line.startswith('@@'): | |
line = stringc(line, C.COLOR_DIFF_LINES) | |
ret.append(line) | |
if has_diff: | |
ret.append('\n') | |
if 'prepared' in diff: | |
ret.append(to_text(diff['prepared'])) | |
except UnicodeDecodeError: | |
ret.append(">> the files are different, but the diff library cannot compare unicode strings\n\n") | |
return u''.join(ret) |
Is it wrong?
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 get it now, so adding diff as part of prepared
key results is rendering diff in a readable format in output logs.
The only concern I have here is this change might break backward compatibility for playbook that depend on diff
key
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.
Ah, that may be the problem.
Do you have any idea to keep backward compatibility?
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.
Note that this line was only added recently (https://github.com/ansible/ansible/pull/41846/files) for Ansible 1.7 development branch, i.e. it has not been added to a release.
So your change should be safe.
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.
In fact, @ganeshrn, you added it a couple of weeks ago ;)
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.
@felixfontein Good catch. Thanks for pointing out :)
@hitsumabushi diff is added as part of result for most network platforms, somehow is was missed in vyos_config for earlier release. I added diff
in vyos_config during the recent cliconf refactor work. Looking at this PR I assumed that it was present since earlier releases.
@hitsumabushi Thank you! |
SUMMARY
ISSUE TYPE
COMPONENT NAME
vyos_config
moduleANSIBLE VERSION
I checked current devel branch. : https://github.com/ansible/ansible/tree/d7c3d5501bdc7482a330fb7b29ad8dd7ed3683b6