Merge pull request #201 from jakub-nt/ENT-10830

ENT-10830: Build steps no longer write duplicate entries to `def.json` where appropriate

JSON.md

@@ -222,7 +222,7 @@ These are copies of the module directories, where it's more "safe" to do things
 ## All available build steps
 
 The build steps below manipulate the temporary files in the steps directories and write results to the output policy set, in `out/masterfiles`.
-Unless otherwise noted, all steps are run inside the module's folder (`out/steps/...`) with sources / file paths relative to that folder, and targets / destinations mentioned below are relative to the output policy set (`out/masterfiles`, which in the end will be deployed as `/var/cfengine/masterfiles`)
+Unless otherwise noted, all steps are run inside the module's folder (`out/steps/...`) with sources / file paths relative to that folder, and targets / destinations mentioned below are relative to the output policy set (`out/masterfiles`, which in the end will be deployed as `/var/cfengine/masterfiles`).
 
 * `copy <source> <destination>`
   * Copy a single file or a directory recursively.
@@ -263,6 +263,14 @@ Unless otherwise noted, all steps are run inside the module's folder (`out/steps
   * Source is relative to module directory and target is relative to `out/masterfiles`.
     * In most cases, the build step should be: `input ./input.json def.json`
 
+When `def.json` is modified during a `json`, `input`, `directory`, `bundles`, or `policy_files` build step, the values of some lists of strings are deduplicated, when this does not make any difference in behavior.
+These cases are:
+
+1. Policy files and augments files in the `"inputs"` and `"augments"` top level keys.
+2. `"tags"` inside variables in `"variables"` and classes in `"classes"`.
+3. Class expressions for each class in `"classes"`.
+   These are in the subkey `"class_expressions"` when the class is defined using an object, and if the class is defined using just a list, that list is the list of class expressions implicitly.
+
 ### A note on reproducibility and backwards compatibility
 
 As mentioned in [the README](./README.md), our main focus when it comes to reproducibility and backwards compatibility of `cfbs` is the `cfbs build` command.

cfbs/build.py

@@ -3,6 +3,7 @@
 from cfbs.utils import (
     canonify,
     cp,
+    deduplicate_def_json,
     find,
     is_valid_arg_count,
     merge_json,
@@ -116,6 +117,8 @@ def _perform_build_step(module, step, max_length):
             print("Warning: '%s' looks empty, adding nothing" % os.path.basename(src))
         if original:
             merged = merge_json(original, extras)
+            if os.path.basename(dst) == "def.json":
+                merged = deduplicate_def_json(merged)
         else:
             merged = extras
         write_json(dst, merged)
@@ -146,6 +149,7 @@ def _perform_build_step(module, step, max_length):
                     extra = read_json(os.path.join(root, f))
                     if extra:
                         merged = merge_json(merged, extra)
+                        merged = deduplicate_def_json(merged)
                 else:
                     s = os.path.join(root, f)
                     d = os.path.join(destination, dstarg, root[len(src) :], f)
@@ -183,6 +187,7 @@ def _perform_build_step(module, step, max_length):
         if original:
             log.debug("Original def.json: %s", pretty(original))
             merged = merge_json(original, extras)
+            merged = deduplicate_def_json(merged)
         else:
             merged = extras
         log.debug("Merged def.json: %s", pretty(merged))
@@ -209,7 +214,11 @@ def _perform_build_step(module, step, max_length):
         path = os.path.join(destination, "def.json")
         original = read_json(path)
         log.debug("Original def.json: %s" % pretty(original))
-        merged = merge_json(original, augment) if original else augment
+        if original:
+            merged = merge_json(original, augment)
+            merged = deduplicate_def_json(merged)
+        else:
+            merged = augment
         log.debug("Merged def.json: %s", pretty(merged))
         write_json(path, merged)
     elif operation == "bundles":
@@ -220,7 +229,11 @@ def _perform_build_step(module, step, max_length):
         path = os.path.join(destination, "def.json")
         original = read_json(path)
         log.debug("Original def.json: %s" % pretty(original))
-        merged = merge_json(original, augment) if original else augment
+        if original:
+            merged = merge_json(original, augment)
+            merged = deduplicate_def_json(merged)
+        else:
+            merged = augment
         log.debug("Merged def.json: %s", pretty(merged))
         write_json(path, merged)
 

cfbs/utils.py

@@ -204,6 +204,38 @@ def merge_json(a, b, overwrite_callback=None, stack=None):
     return a
 
 
+def deduplicate_def_json(d):
+    if "inputs" in d:
+        d["inputs"] = deduplicate_list(d["inputs"])
+    if "augments" in d:
+        d["augments"] = deduplicate_list(d["augments"])
+
+    for variable in d.get("variables", {}).values():
+        if type(variable) is not dict:
+            continue
+        if "tags" in variable:
+            variable["tags"] = deduplicate_list(variable["tags"])
+
+    for class_name, class_v in d.get("classes", {}).items():
+        if type(class_v) is dict:
+            if "class_expressions" in class_v:
+                class_v["class_expressions"] = deduplicate_list(
+                    class_v["class_expressions"]
+                )
+            if "tags" in class_v:
+                class_v["tags"] = deduplicate_list(class_v["tags"])
+        elif type(class_v) is list:
+            d["classes"][class_name] = deduplicate_list(class_v)
+
+    # TODO: "vars" can have "augments_inputs", perhaps it could be deduplicated too
+
+    return d
+
+
+def deduplicate_list(l):
+    return list(OrderedDict.fromkeys(l))
+
+
 def cfbs_filename() -> str:
     return "cfbs.json"
 

tests/test_utils.py

@@ -1,4 +1,4 @@
-from cfbs.utils import canonify, merge_json, loads_bundlenames
+from cfbs.utils import canonify, deduplicate_def_json, merge_json, loads_bundlenames
 
 
 def test_canonify():
@@ -55,6 +55,91 @@ def test_merge_json():
     assert merged == expected
 
 
+def test_deduplicate_def_json():
+    case = {
+        "inputs": [
+            "services/cfbs/inventory/company.cf",
+            "services/cfbs/inventory/company.cf",
+            "services/cfbs/inventory/company.cf",
+        ]
+    }
+    expected = {"inputs": ["services/cfbs/inventory/company.cf"]}
+
+    deduplicated = deduplicate_def_json(case)
+    assert deduplicated == expected
+
+    case = {
+        "augments": [
+            "/var/cfengine/augments/company.json",
+            "/var/cfengine/augments/company.json",
+        ],
+        "variables": {
+            "MyNamespace:my_bundle.Variable": {
+                "value": {"tags": ["dont-dedupe", "dont-dedupe"]},
+                "tags": ["inventory", "attribute_name=My Inventory", "inventory"],
+            }
+        },
+    }
+    expected = {
+        "augments": ["/var/cfengine/augments/company.json"],
+        "variables": {
+            "MyNamespace:my_bundle.Variable": {
+                "value": {"tags": ["dont-dedupe", "dont-dedupe"]},
+                "tags": ["inventory", "attribute_name=My Inventory"],
+            }
+        },
+    }
+
+    deduplicated = deduplicate_def_json(case)
+    assert deduplicated == expected
+
+    case = {
+        "classes": {
+            "my-class": [
+                "^(?!MISSING).*",
+                "cfengine::",
+                "^(?!MISSING).*",
+                "cfengine::",
+            ],
+        },
+        "vars": {"augments_inputs": ["dont-dedupe-for-now", "dont-dedupe-for-now"]},
+    }
+    expected = {
+        "classes": {
+            "my-class": [
+                "^(?!MISSING).*",
+                "cfengine::",
+            ],
+        },
+        "vars": {"augments_inputs": ["dont-dedupe-for-now", "dont-dedupe-for-now"]},
+    }
+
+    deduplicated = deduplicate_def_json(case)
+    assert deduplicated == expected
+
+    case = {
+        "classes": {
+            "my-class": {
+                "class_expressions": ["cfengine|linux::", "cfengine|linux::"],
+                "comment": "Optional class description of class",
+                "tags": ["tags", "tags"],
+            },
+        },
+    }
+    expected = {
+        "classes": {
+            "my-class": {
+                "class_expressions": ["cfengine|linux::"],
+                "comment": "Optional class description of class",
+                "tags": ["tags"],
+            },
+        },
+    }
+
+    deduplicated = deduplicate_def_json(case)
+    assert deduplicated == expected
+
+
 def test_loads_bundlenames_single_bundle():
     policy = """bundle agent bogus
 {