WIP: New module: diff (string, file or command output vs string, file or command output)

[cytopia]

SUMMARY

This module lets you diff compare a string, a file or a command output against any combination of another string, file or command output. If the output differs (changes are present), then Ansible will report its state as changed, otherwise not.

Check mode is supported, except when diffing a command. In this case it will automatically skip the task with a custom skip message.

ISSUE TYPE

• New Module Pull Request

COMPONENT NAME

diff

ANSIBLE VERSION

ansible 2.3.2.0
  config file =
  configured module search path = Default w/o overrides
  python version = 3.5.3 (default, Jan 19 2017, 14:11:04) [GCC 6.3.0 20170118]

ADDITIONAL INFORMATION

For reference see here: https://github.com/cytopia/ansible-modules

TASK [my_diff : diff] ****************************************************************************************
task path: /home/cytopia/repo/ansible/roles/my_diff/tasks/main.yml:37
--- before
+++ after
@@ -1,4 +1,4 @@
-local_parameters:
+global_parameters:
     param1: foo
     param:2 bar
@@ -72,3 +72,4 @@

     some line
     some other line
+

changed: [my_diff] => {"changed": true}

[ansibot]

The test ansible-test sanity --test ansible-doc --python 2.6 failed with the following error:

Output on stderr from ansible-doc is considered an error.

Command "ansible-doc diff" returned exit status 0.
>>> Standard Error
[WARNING]: While constructing a mapping from
/root/src/github.com/ansible/ansible/lib/ansible/modules/files/diff.py, line
13, column 5, found a duplicate dict key (target). Using last defined value
only.

The test ansible-test sanity --test ansible-doc --python 2.7 failed with the following error:

Output on stderr from ansible-doc is considered an error.

Command "ansible-doc diff" returned exit status 0.
>>> Standard Error
[WARNING]: While constructing a mapping from
/root/src/github.com/ansible/ansible/lib/ansible/modules/files/diff.py, line
13, column 5, found a duplicate dict key (target). Using last defined value
only.

The test ansible-test sanity --test ansible-doc --python 3.5 failed with the following error:

Output on stderr from ansible-doc is considered an error.

Command "ansible-doc diff" returned exit status 0.
>>> Standard Error
[WARNING]: While constructing a mapping from
/root/src/github.com/ansible/ansible/lib/ansible/modules/files/diff.py, line
13, column 5, found a duplicate dict key (target). Using last defined value
only.

The test ansible-test sanity --test ansible-doc --python 3.6 failed with the following error:

Output on stderr from ansible-doc is considered an error.

Command "ansible-doc diff" returned exit status 0.
>>> Standard Error
[WARNING]: While constructing a mapping from
/root/src/github.com/ansible/ansible/lib/ansible/modules/files/diff.py, line
13, column 5, found a duplicate dict key (target). Using last defined value
only.

The test ansible-test sanity --test boilerplate failed with the following error:

Command "test/sanity/code-smell/boilerplate.sh" returned exit status 2.
>>> Standard Output
== Missing __metaclass__ = type ==
./lib/ansible/modules/files/diff.py

== Missing from __future__ import (absolute_import, division, print_function) ==
./lib/ansible/modules/files/diff.py

The test ansible-test sanity --test validate-modules failed with the following error:

lib/ansible/modules/files/diff.py:0:0: E316 ANSIBLE_METADATA.metadata_version: not a valid value for dictionary value @ data['metadata_version']. Got '1.0'

click here for bot help

[ansibot]

The test ansible-test sanity --test boilerplate failed with the following error:

Command "test/sanity/code-smell/boilerplate.sh" returned exit status 2.
>>> Standard Output
== Missing __metaclass__ = type ==
./lib/ansible/modules/files/diff.py

== Missing from __future__ import (absolute_import, division, print_function) ==
./lib/ansible/modules/files/diff.py

The test ansible-test sanity --test validate-modules failed with the following error:

lib/ansible/modules/files/diff.py:0:0: E316 ANSIBLE_METADATA.metadata_version: not a valid value for dictionary value @ data['metadata_version']. Got '1.0'

click here for bot help

[ansibot]

@EvanK @ahtik @astorije @bendoh @bpennypacker @cmprescott @dhozac @jhoekx @jirutka @jpmens @luisperlaz @noseka1 @pileofrogs @ribbons @sfromm @sm4rk0 @tbielawa @tima @yaegashi

As a maintainer of a module in the same namespace this new module has been submitted to, your vote counts for shipits. Please review this module and add shipit if you would like to see it merged.

click here for bot help

[dagwieers]

Weird, I proposed this module less than 24h ago on #ansible-devel, and implemented my own version. The version I propose is pure-python.

Update: I made a PR of my version at: #28475

Your module does more than I needed, I only needed to compare a file to another file (or string). and I wanted it to be pure python (so you could compare remotely on systems that have python, but may not have diff) using difflib. Maybe you can integrate this into your module ?

[dagwieers]

So I would change the user-interface of the module so that you can do e.g.:

- diff:
    src_file: /foo/bar
    dest_string: |
      foo: bar
      foob: blab

- diff:
    src_cmd: iptables -L -xnv
    dest_file: iptables-output.txt

I would also implement remote_src so that the user can decide whether a src file or command originates from the control machine, or the remote node.

[dagwieers]

The author part is a list now, so it should be:

author:
- cytopia (@cytopia)

[dagwieers]

We now have a one-line GPL header that we use:

# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)

[dagwieers]

Please don't add default: null, that's implicit and adds no value. (Especially since it is a required parameter)

[dagwieers]

Please don't add default: null, that's implicit and adds no value. (Especially since it is a required parameter)

[dagwieers]

We do not typically have a parameter that can be one of many things. Mostly because it makes the argument_spec hard to design. What you should be doing here is make different (mutual exclusive) parameters for a string, file or command using mutual_exclusive and required_together. and then you don't need a type selector parameter, the used options will make clear what the user wants.

[dagwieers]

Empty aliases is not needed. Keep it simple.

[dagwieers]

Empty aliases is not needed. Keep it simple.

[dagwieers]

So I would get rid of these.

[cytopia]

@dagwieers thanks for the review. I will take this into account.

[cytopia]

@dagwieers

We do not typically have a parameter that can be one of many things. Mostly because it makes the argument_spec hard to design. What you should be doing here is make different (mutual exclusive) parameters for a string, file or command using mutual_exclusive and required_together. and then you don't need a type selector parameter, the used options will make clear what the user wants.

I understand, however then it might also get messy:

    module = AnsibleModule(
        argument_spec=dict(
            src_string=dict(required=False, default=None, type='str'),
            src_file_remote=dict(required=False, default=None, type='path'),
            src_file_locale=dict(required=False, default=None, type='path'),
            src_cmd_remote=dict(required=False, default=None, type='str'),
            src_cmd_local=dict(required=False, default=None, type='str'),
            dst_string=dict(required=False, default=None, type='str'),
            dst_file_remote=dict(required=False, default=None, type='path'),
            dst_file_locale=dict(required=False, default=None, type='path'),
            dst_cmd_remote=dict(required=False, default=None, type='str'),
            dst_cmd_local=dict(required=False, default=None, type='str')
        ),
        supports_check_mode=True,
        mutually_exclusive=[
            ['src_string', 'src_file_remote', 'src_file_local', 'src_cmd_remote', 'src_cmd_local'],
            ['dst_string', 'dst_file_remote', 'dst_file_local', 'dst_cmd_remote', 'dst_cmd_local']
        ]
    )

(btw, how would I do the required_together on the above ones?)

As opposed to the following, which would allow to easily add more options wihtout introducing more parameters:

    module = AnsibleModule(
        argument_spec=dict(
            source=dict(required=True, default=None, type='str'),
            target=dict(required=True, default=None, type='str'),
            source_type=dict(required=True, default='string',
                             choices=['string', 'local_file', 'remote_file',
                                      'local_cmd', 'remote_cmd']),
            target_type=dict(required=True, default='string',
                             choices=['string', 'local_file', 'remote_file',
                                      'local_cmd', 'remote_cmd']),
        ),
        supports_check_mode=True
    )

What do you think?

[dagwieers]

You don't need local/remote, the option remote_src exists in various Ansible modules already. And you don't need to support the case where both files are local (because why not run it on localhost in that case).

You have 3 options for both src and dest. string, file and command (although you could only support string and file and leave command to Ansible tasks). And only src could come from the control host (and by default the src is always to be expected to come from the control host)

So in the most simple case, I would go for:

    module = AnsibleModule(
        argument_spec=dict(
            src_string=dict(type='str', aliases=['content']),
            src_file=dict(type='path', aliases=['src']),
            dest_string=dict(type='str'),
            dest_file=dict(type='path', aliases=['dest']),
            remote_src=dict(type='bool', default=False),
        ),
        supports_check_mode=True,
        mutually_exclusive=[
            ['src_file', 'src_string'],
            ['dest_file', 'dest_string'],
        ],
        required_one_of=[
            ['src_file', 'src_string'],
            ['dest_file', 'dest_string'],
        ],
    )

Beware that:

• I removed all the unwanted default options from the argument_spec. (I always specify the type first)
• Since the standard parameters are src and dest I prefer we use these and not dst
• Since src and dest are files usually, I opted to make these aliases too
• There is also an alternative to src for inline content, named content I made that an alias too
• I always add a trailing comma when listing multi-line dict-values or list-values, so it's easy to add entries or change order (this is specifically allowed by PEP8 for convenience)
• I would use difflib for generating diff output (this is going to make it more portable, think Windows/python)
• We have to be careful that the diff's are not becoming too large, so either truncate the final output to a specific size, see git module diff mode is excessively verbose  #28319

[cytopia]

@dagwieers the whole point of this PR was that I will be able to diff a command output against a string or a file on a remote host (where the role is run against). This is possible with the current code.

Introducing your changes for local and remote support:

Now, I could go further and add support for both, local and remote support (but not remove command output support, as this is what I require). This would allow for 25 different combinations:

Where X could be:

• string
• local file
• remote file
• local command
• remote command

left	right
X	string
X	local file
X	remote file
X	local command
X	remote command

So I do need those parameters:

            src_string=dict(required=False, default=None, type='str'),
            src_file_remote=dict(required=False, default=None, type='path'),
            src_file_locale=dict(required=False, default=None, type='path'),
            src_cmd_remote=dict(required=False, default=None, type='str'),
            src_cmd_local=dict(required=False, default=None, type='str'),
            dst_string=dict(required=False, default=None, type='str'),
            dst_file_remote=dict(required=False, default=None, type='path'),
            dst_file_locale=dict(required=False, default=None, type='path'),
            dst_cmd_remote=dict(required=False, default=None, type='str'),
            dst_cmd_local=dict(required=False, default=None, type='str')

They can also be renamed. From what you have posted, I see that you are using the terms src and dest for local and remote as well as for left and right . So your left side of the diff is always local and your right side of the diff is always remote. You wouldn't be able to compare two remote files.

So I guess proper naming must be defined that it allows all 25 combinations listed above.

The only two questions I still have is:

• How can I execute a command locally or remotely?
• How can I read a file from either local or remote?

[dagwieers]

• Well, the question is "do you really want the diff command to run commands ?". We have modules to run commands, which are much more versatile than adding the functionality to diff. You may want to run the command as a different user, or in a different directory, or on another host, maybe you want to run scripts not a command, etc... So I don't think "running commands" is something the diff module will be compelling at.

• The other item you seem to be ignoring is the fact that the remote_src option is there to tell the module that the src is located on the remote machine. This is a standard practice in Ansible and we should be using the same standard practices.

How can I execute a command locally or remotely?

- command: echo Remote command
  register: remote_cmd
- command: echo Local command as root
  register: local_root_cmd
  become_user: root
  delegate_to: localhost
- diff:
    src_string: '{{ remote_cmd.stdout }}'
    dest_string: '{{ local_root_cmd.stdout }}'

NOTE: The command could be run on a second host, or a reference host, it doesn't really matter because you are passing strings. Any string you like. And this makes it possible to also compare stderr, if you like.

How can I read a file from either local or remote?

- name: Compare local and remote file
  diff:
    src: /some/local/file.txt
    dest: /some/remote/file.txt
- name: Compare 2 remote files
  diff:
    src: /some/remote/file1.txt
    dest: /some/remote/file2.txt
    remote_src: yes
- name: Compare 2 local files:
  diff:
    src: /some/local/file1.txt
    dest: /some/local/file2.txt
  delegate_to: localhost

How to compare a command to an expected string?

- command: echo Foobar
  register: foobar
- diff:
    src_string: Foobar
    dest_string: '{{ foobar.stdout }}'

- diff:
    content: foobar.ansible.com
    dest: /etc/hostname

[bcoca]

it seems simpler to have a filter or lookup that does this, in it's simplest form:
'stringa'|diff('stringb')

but then you can use variables and/or lookups to deal with remote/local files:
lookup('file', '/path/to/file')|diff(reg_slurp.content)

but for files, in most cases i would just use 'copy' in diff mode, with the 'content' option for when you have a string when running with --diff:

 - copy: src=/path/file1 dest=/path/file2

Soon we should have 'per task' diff: no|yes optoins #28581

[cytopia]

What about the case I want to view the difference for two remote files? Such as:

• /etc/nginx/nginx.conf
• /etc/nginx/nginx.conf.rpmnew

My above suggestion (with many cases) would cover that as well.

[bcoca]

A diff filter still covers that case as you can fetch/slurp both files

[ansibot]

@cytopia Your branch does not contain a shippable.yml file. Please rebase your branch to trigger running of current tests.

click here for bot help

[gundalow]

@cytopia
Thank you for your PR.
As part of reviewing the backlog of PRs we are looking at PRs older PRs that haven't been updated in a while

Given that:

• It's remained WIP (Work in progress) since 2017
• This is a feature rather than a bug fix
• The source branch (in your fork) no longer exists

Therefore I'm going to close this.

If you or anyone else wants to continue with this work then please do feel free to create a fresh PR and @mention the previous reviewers in here.