Changed always true for some reason

[e-g1gor]

I'm trying to write task to change docker config, and restart docker service only if it was changed - and json_patch task for some reason always return changed: True

Return:

{
  "changed": false,
  "dest": "/etc/docker/daemon.json",
  "diff": {
    "after": "{\n    \"log-driver\": \"json-file\",\n    \"log-opts\": {\n        \"max-size\": \"10m\",\n        \"max-file\": \"10\"\n    }\n}",
    "after_header": "/etc/docker/daemon.json (content)",
    "before": "{\n    \"log-driver\": \"json-file\",\n    \"log-opts\": {\n        \"max-size\": \"10m\",\n        \"max-file\": \"10\"\n    }\n}",
    "before_header": "/etc/docker/daemon.json (content)"
  },
  "failed": false,
  "gid": 0,
  "group": "root",
  "mode": "0644",
  "owner": "root",
  "secontext": "system_u:object_r:container_config_t:s0",
  "size": 110,
  "state": "file",
  "uid": 0
}

Task example:

    - name: Configure docker
      become: true
      block:
        - name: Setup docker logrotation by default
          json_patch:
            src: "/etc/docker/daemon.json"
            operations:
              - op: add
                path: "/log-driver"
                value: "json-file"
              - op: add
                path: "/log-opts"
                value: {}
              - op: add
                path: "/log-opts/max-size"
                value: "10m"
              - op: add
                path: "/log-opts/max-file"
                value: "10"
            pretty: true
            create: true
          register: json_patch_output
          changed_when: json_patch_output.diff.before != json_patch_output.diff.after

        - name: Set is_docker_config_changed
          ansible.builtin.set_fact:
            is_docker_config_changed: "{{ json_patch_output.diff.before != json_patch_output.diff.after }}"

        - name: Restart docker
          when: is_docker_config_changed
          ansible.builtin.systemd:
            name: docker
            state: restarted

[verschmelzen]

According to RFC the following removes initial config if it is present, thus considreing operation changed

...
              - op: add
                path: "/log-opts"
                value: {}
...

In that regard the RFC is not really fit for configuration management where we need to ensure presence of deep values without unnecessary resets (what if /log-opts had other properties that we don't want to touch?).

So we can just ignore the RFC and make def add work like mkdir -p, basically change one line

--- json_patch.py
+++ json_patch.py
@@ -403,7 +403,7 @@
                 try:
                     next_obj = obj[path]
                 except KeyError:
-                    raise PathError("could not find '%s' member in JSON object" % path)
+                    next_obj = obj[path] = {}
                 obj[path], chg, _ = self.add(remaining, value, next_obj)
             elif isinstance(obj, list):
                 if not path.isdigit()

[tnyeanderson]

I don't like ignoring RFCs, but I just hit this same problem, and you're right that it is very unhelpful for config files.

Instead of changing the behavior in an unexpected way (for those who want the RFC followed by default, which is probably the correct thing to do), I propose one or more of the following:

1. An additional boolean in each operation which can control this. Something like the below, but probably with a better name:

   operations:
     - op: add
       path: /parent/child
       value: myval
       createMissingParents: true
   

2. A task-level option to control this:

   createMissingParents: true
   operations:
     - op: add
       path: /parent/child
       value: myval
   

3. An entirely new operation which accomplishes this:

   operations:
     - op: addp
       path: /parent/child
       value: myval
   

Since I think option 1 is the most useful and clear, I'm going to get a PR going for it. Should be in by tomorrow and we can continue discussing there :)

[tnyeanderson]

PR submitted. To ease documentation and avoid confusion, I went with option 2 instead. Hopefully @particledecay will have some time to take a look :)