From 026416d158baef451240c0d021e4b47b0e188e56 Mon Sep 17 00:00:00 2001 From: Miguel Jacq Date: Tue, 16 Dec 2025 20:48:08 +1100 Subject: [PATCH] Fix tests --- enroll/harvest.py | 2 +- enroll/manifest.py | 58 +++++++++++++++---- tests.sh | 2 +- tests/test_cli.py | 56 ++++++++++++++++-- tests/test_jinjaturtle.py | 99 ++++++++++++++++++++++++++++++++ tests/test_manifest.py | 118 ++++++++++++++++++++++++++++++++++++-- 6 files changed, 313 insertions(+), 22 deletions(-) create mode 100644 tests/test_jinjaturtle.py diff --git a/enroll/harvest.py b/enroll/harvest.py index 78f7d1f..688a489 100644 --- a/enroll/harvest.py +++ b/enroll/harvest.py @@ -132,7 +132,7 @@ def _safe_name(s: str) -> str: def _role_id(raw: str) -> str: - # normalize separators first + # normalise separators first s = re.sub(r"[^A-Za-z0-9]+", "_", raw) # split CamelCase -> snake_case s = re.sub(r"([a-z0-9])([A-Z])", r"\1_\2", s) diff --git a/enroll/manifest.py b/enroll/manifest.py index e27cfd5..7565160 100644 --- a/enroll/manifest.py +++ b/enroll/manifest.py @@ -48,8 +48,24 @@ def _yaml_dump_mapping(obj: Dict[str, Any], *, sort_keys: bool = True) -> str: for k, v in sorted(obj.items()) if sort_keys else obj.items(): lines.append(f"{k}: {v!r}") return "\n".join(lines).rstrip() + "\n" + + # ansible-lint/yamllint's indentation rules are stricter than YAML itself. + # In particular, they expect sequences nested under a mapping key to be + # indented (e.g. `foo:\n - a`), whereas PyYAML's default is often + # `foo:\n- a`. + class _IndentDumper(yaml.SafeDumper): # type: ignore + def increase_indent(self, flow: bool = False, indentless: bool = False): + return super().increase_indent(flow, False) + return ( - yaml.safe_dump(obj, default_flow_style=False, sort_keys=sort_keys).rstrip() + yaml.dump( + obj, + Dumper=_IndentDumper, + default_flow_style=False, + sort_keys=sort_keys, + indent=2, + allow_unicode=True, + ).rstrip() + "\n" ) @@ -124,7 +140,7 @@ def _extract_jinjaturtle_block(text: str) -> str: return text.strip() + "\n" -def _normalize_jinjaturtle_vars_text(vars_text: str) -> str: +def _normalise_jinjaturtle_vars_text(vars_text: str) -> str: """Deduplicate keys in a vars fragment by parsing as YAML and dumping it back.""" m = _yaml_load_mapping(vars_text) if not m: @@ -166,14 +182,14 @@ def _copy_artifacts( dst = os.path.join(dst_files_dir, rel) # If a file was successfully templatised by JinjaTurtle, do NOT - # also materialize the raw copy in the destination files dir. + # also materialise the raw copy in the destination files dir. # (This keeps the output minimal and avoids redundant "raw" files.) if exclude_rels and rel in exclude_rels: try: if os.path.isfile(dst): os.remove(dst) except Exception: - pass + pass # nosec continue if preserve_existing and os.path.exists(dst): @@ -342,7 +358,7 @@ def _jinjify_managed_files( except Exception: # If jinjaturtle cannot process a file for any reason, skip silently. # (Enroll's core promise is to be optimistic and non-interactive.) - continue + continue # nosec tmpl_rel = src_rel + ".j2" tmpl_dst = os.path.join(role_dir, "templates", tmpl_rel) @@ -372,7 +388,7 @@ def _hostvars_only_jinjaturtle(vars_text: str) -> str: def _defaults_with_jinjaturtle(base_defaults: str, vars_text: str) -> str: if not vars_text.strip(): return base_defaults.rstrip() + "\n" - vars_text = _normalize_jinjaturtle_vars_text(vars_text) + vars_text = _normalise_jinjaturtle_vars_text(vars_text) # Always regenerate the block (we regenerate whole defaults files anyway) return ( base_defaults.rstrip() @@ -450,7 +466,11 @@ def _render_generic_files_tasks( owner: "{{{{ item.owner }}}}" group: "{{{{ item.group }}}}" mode: "{{{{ item.mode }}}}" - loop: "{{{{ {var_prefix}_managed_files | default([]) | selectattr('is_systemd_unit','equalto', true) | selectattr('kind','equalto','template') | list }}}}" + loop: >- + {{{{ {var_prefix}_managed_files | default([]) + | selectattr('is_systemd_unit', 'equalto', true) + | selectattr('kind', 'equalto', 'template') + | list }}}} notify: "{{{{ item.notify | default([]) }}}}" - name: Deploy systemd unit files (copies) @@ -465,12 +485,20 @@ def _render_generic_files_tasks( owner: "{{{{ item.owner }}}}" group: "{{{{ item.group }}}}" mode: "{{{{ item.mode }}}}" - loop: "{{{{ {var_prefix}_managed_files | default([]) | selectattr('is_systemd_unit','equalto', true) | selectattr('kind','equalto','copy') | list }}}}" + loop: >- + {{{{ {var_prefix}_managed_files | default([]) + | selectattr('is_systemd_unit', 'equalto', true) + | selectattr('kind', 'equalto', 'copy') + | list }}}} notify: "{{{{ item.notify | default([]) }}}}" - name: Reload systemd to pick up unit changes ansible.builtin.meta: flush_handlers - when: "({var_prefix}_managed_files | default([]) | selectattr('is_systemd_unit','equalto', true) | list | length) > 0" + when: >- + ({var_prefix}_managed_files | default([]) + | selectattr('is_systemd_unit', 'equalto', true) + | list + | length) > 0 - name: Deploy other managed files (templates) ansible.builtin.template: @@ -479,7 +507,11 @@ def _render_generic_files_tasks( owner: "{{{{ item.owner }}}}" group: "{{{{ item.group }}}}" mode: "{{{{ item.mode }}}}" - loop: "{{{{ {var_prefix}_managed_files | default([]) | selectattr('is_systemd_unit','equalto', false) | selectattr('kind','equalto','template') | list }}}}" + loop: >- + {{{{ {var_prefix}_managed_files | default([]) + | selectattr('is_systemd_unit', 'equalto', false) + | selectattr('kind', 'equalto', 'template') + | list }}}} notify: "{{{{ item.notify | default([]) }}}}" - name: Deploy other managed files (copies) @@ -494,7 +526,11 @@ def _render_generic_files_tasks( owner: "{{{{ item.owner }}}}" group: "{{{{ item.group }}}}" mode: "{{{{ item.mode }}}}" - loop: "{{{{ {var_prefix}_managed_files | default([]) | selectattr('is_systemd_unit','equalto', false) | selectattr('kind','equalto','copy') | list }}}}" + loop: >- + {{{{ {var_prefix}_managed_files | default([]) + | selectattr('is_systemd_unit', 'equalto', false) + | selectattr('kind', 'equalto', 'copy') + | list }}}} notify: "{{{{ item.notify | default([]) }}}}" """ diff --git a/tests.sh b/tests.sh index f8d246c..ea7ad59 100755 --- a/tests.sh +++ b/tests.sh @@ -11,7 +11,7 @@ rm -rf "${BUNDLE_DIR}" "${ANSIBLE_DIR}" # Generate data poetry run \ - enroll enroll \ + enroll single-shot \ --harvest "${BUNDLE_DIR}" \ --out "${ANSIBLE_DIR}" diff --git a/tests/test_cli.py b/tests/test_cli.py index a93c509..9e3422c 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -22,9 +22,12 @@ def test_cli_harvest_subcommand_calls_harvest(monkeypatch, capsys, tmp_path): def test_cli_manifest_subcommand_calls_manifest(monkeypatch, tmp_path): called = {} - def fake_manifest(harvest_dir: str, out_dir: str): + def fake_manifest(harvest_dir: str, out_dir: str, **kwargs): called["harvest"] = harvest_dir called["out"] = out_dir + # Common manifest args should be passed through by the CLI. + called["fqdn"] = kwargs.get("fqdn") + called["jinjaturtle"] = kwargs.get("jinjaturtle") monkeypatch.setattr(cli, "manifest", fake_manifest) monkeypatch.setattr( @@ -43,6 +46,8 @@ def test_cli_manifest_subcommand_calls_manifest(monkeypatch, tmp_path): cli.main() assert called["harvest"] == str(tmp_path / "bundle") assert called["out"] == str(tmp_path / "ansible") + assert called["fqdn"] is None + assert called["jinjaturtle"] == "auto" def test_cli_enroll_subcommand_runs_harvest_then_manifest(monkeypatch, tmp_path): @@ -52,8 +57,16 @@ def test_cli_enroll_subcommand_runs_harvest_then_manifest(monkeypatch, tmp_path) calls.append(("harvest", bundle_dir)) return str(tmp_path / "bundle" / "state.json") - def fake_manifest(bundle_dir: str, out_dir: str): - calls.append(("manifest", bundle_dir, out_dir)) + def fake_manifest(bundle_dir: str, out_dir: str, **kwargs): + calls.append( + ( + "manifest", + bundle_dir, + out_dir, + kwargs.get("fqdn"), + kwargs.get("jinjaturtle"), + ) + ) monkeypatch.setattr(cli, "harvest", fake_harvest) monkeypatch.setattr(cli, "manifest", fake_manifest) @@ -62,7 +75,7 @@ def test_cli_enroll_subcommand_runs_harvest_then_manifest(monkeypatch, tmp_path) "argv", [ "enroll", - "enroll", + "single-shot", "--harvest", str(tmp_path / "bundle"), "--out", @@ -73,5 +86,38 @@ def test_cli_enroll_subcommand_runs_harvest_then_manifest(monkeypatch, tmp_path) cli.main() assert calls == [ ("harvest", str(tmp_path / "bundle")), - ("manifest", str(tmp_path / "bundle"), str(tmp_path / "ansible")), + ("manifest", str(tmp_path / "bundle"), str(tmp_path / "ansible"), None, "auto"), ] + + +def test_cli_manifest_common_args(monkeypatch, tmp_path): + """Ensure --fqdn and jinjaturtle mode flags are forwarded correctly.""" + + called = {} + + def fake_manifest(harvest_dir: str, out_dir: str, **kwargs): + called["harvest"] = harvest_dir + called["out"] = out_dir + called["fqdn"] = kwargs.get("fqdn") + called["jinjaturtle"] = kwargs.get("jinjaturtle") + + monkeypatch.setattr(cli, "manifest", fake_manifest) + monkeypatch.setattr( + sys, + "argv", + [ + "enroll", + "manifest", + "--harvest", + str(tmp_path / "bundle"), + "--out", + str(tmp_path / "ansible"), + "--fqdn", + "example.test", + "--no-jinjaturtle", + ], + ) + + cli.main() + assert called["fqdn"] == "example.test" + assert called["jinjaturtle"] == "off" diff --git a/tests/test_jinjaturtle.py b/tests/test_jinjaturtle.py new file mode 100644 index 0000000..68bb04c --- /dev/null +++ b/tests/test_jinjaturtle.py @@ -0,0 +1,99 @@ +import json +from pathlib import Path + +import enroll.manifest as manifest_mod +from enroll.jinjaturtle import JinjifyResult + + +def test_manifest_uses_jinjaturtle_templates_and_does_not_copy_raw( + monkeypatch, tmp_path: Path +): + """If jinjaturtle can templatisize a file, we should store a template in the role + and avoid keeping the raw file copy in the destination files area. + + This test stubs out jinjaturtle execution so it doesn't depend on the external tool. + """ + + bundle = tmp_path / "bundle" + out = tmp_path / "ansible" + + # A jinjaturtle-compatible config file. + (bundle / "artifacts" / "foo" / "etc").mkdir(parents=True, exist_ok=True) + (bundle / "artifacts" / "foo" / "etc" / "foo.ini").write_text( + "[main]\nkey = 1\n", encoding="utf-8" + ) + + state = { + "host": {"hostname": "test", "os": "debian"}, + "users": { + "role_name": "users", + "users": [], + "managed_files": [], + "excluded": [], + "notes": [], + }, + "etc_custom": { + "role_name": "etc_custom", + "managed_files": [], + "excluded": [], + "notes": [], + }, + "services": [ + { + "unit": "foo.service", + "role_name": "foo", + "packages": ["foo"], + "active_state": "inactive", + "sub_state": "dead", + "unit_file_state": "disabled", + "condition_result": "no", + "managed_files": [ + { + "path": "/etc/foo.ini", + "src_rel": "etc/foo.ini", + "owner": "root", + "group": "root", + "mode": "0644", + "reason": "modified_conffile", + } + ], + "excluded": [], + "notes": [], + } + ], + "package_roles": [], + } + + bundle.mkdir(parents=True, exist_ok=True) + (bundle / "state.json").write_text(json.dumps(state, indent=2), encoding="utf-8") + + # Pretend jinjaturtle exists. + monkeypatch.setattr( + manifest_mod, "find_jinjaturtle_cmd", lambda: "/usr/bin/jinjaturtle" + ) + + # Stub jinjaturtle output. + def fake_run_jinjaturtle( + jt_exe: str, src_path: str, *, role_name: str, force_format=None + ): + assert role_name == "foo" + return JinjifyResult( + template_text="[main]\nkey = {{ foo_key }}\n", + vars_text="foo_key: 1\n", + ) + + monkeypatch.setattr(manifest_mod, "run_jinjaturtle", fake_run_jinjaturtle) + + manifest_mod.manifest(str(bundle), str(out), jinjaturtle="on") + + # Template should exist in the role. + assert (out / "roles" / "foo" / "templates" / "etc" / "foo.ini.j2").exists() + + # Raw file should NOT be copied into role files/ because it was templatised. + assert not (out / "roles" / "foo" / "files" / "etc" / "foo.ini").exists() + + # Defaults should include jinjaturtle vars. + defaults = (out / "roles" / "foo" / "defaults" / "main.yml").read_text( + encoding="utf-8" + ) + assert "foo_key: 1" in defaults diff --git a/tests/test_manifest.py b/tests/test_manifest.py index 09c66e1..98f418f 100644 --- a/tests/test_manifest.py +++ b/tests/test_manifest.py @@ -94,10 +94,16 @@ def test_manifest_writes_roles_and_playbook_with_clean_when(tmp_path: Path): manifest(str(bundle), str(out)) - # Service role: conditional start must be a clean Ansible expression + # Service role: systemd management should be gated on foo_manage_unit and a probe. tasks = (out / "roles" / "foo" / "tasks" / "main.yml").read_text(encoding="utf-8") - assert "when:\n - _unit_probe is succeeded\n - foo_start | bool\n" in tasks - # Ensure we didn't emit deprecated/broken '{{ }}' delimiters in when: + assert "- name: Probe whether systemd unit exists and is manageable" in tasks + assert "when: foo_manage_unit | default(false)" in tasks + assert ( + "when:\n - foo_manage_unit | default(false)\n - _unit_probe is succeeded\n" + in tasks + ) + + # Ensure we didn't emit deprecated/broken '{{ }}' delimiters in when: lines. for line in tasks.splitlines(): if line.lstrip().startswith("when:"): assert "{{" not in line and "}}" not in line @@ -105,7 +111,9 @@ def test_manifest_writes_roles_and_playbook_with_clean_when(tmp_path: Path): defaults = (out / "roles" / "foo" / "defaults" / "main.yml").read_text( encoding="utf-8" ) - assert "foo_start: false" in defaults + assert "foo_manage_unit: true" in defaults + assert "foo_systemd_enabled: true" in defaults + assert "foo_systemd_state: stopped" in defaults # Playbook should include users, etc_custom, packages, and services pb = (out / "playbook.yml").read_text(encoding="utf-8") @@ -113,3 +121,105 @@ def test_manifest_writes_roles_and_playbook_with_clean_when(tmp_path: Path): assert "- etc_custom" in pb assert "- curl" in pb assert "- foo" in pb + + +def test_manifest_site_mode_creates_host_inventory_and_raw_files(tmp_path: Path): + """In --fqdn mode, host-specific state goes into inventory/host_vars.""" + + fqdn = "host1.example.test" + bundle = tmp_path / "bundle" + out = tmp_path / "ansible" + + # Artifacts for a service-managed file. + (bundle / "artifacts" / "foo" / "etc").mkdir(parents=True, exist_ok=True) + (bundle / "artifacts" / "foo" / "etc" / "foo.conf").write_text( + "x", encoding="utf-8" + ) + + # Artifacts for etc_custom file so copy works. + (bundle / "artifacts" / "etc_custom" / "etc" / "default").mkdir( + parents=True, exist_ok=True + ) + (bundle / "artifacts" / "etc_custom" / "etc" / "default" / "keyboard").write_text( + "kbd", encoding="utf-8" + ) + + state = { + "host": {"hostname": "test", "os": "debian"}, + "users": { + "role_name": "users", + "users": [], + "managed_files": [], + "excluded": [], + "notes": [], + }, + "etc_custom": { + "role_name": "etc_custom", + "managed_files": [ + { + "path": "/etc/default/keyboard", + "src_rel": "etc/default/keyboard", + "owner": "root", + "group": "root", + "mode": "0644", + "reason": "custom_unowned", + } + ], + "excluded": [], + "notes": [], + }, + "services": [ + { + "unit": "foo.service", + "role_name": "foo", + "packages": ["foo"], + "active_state": "active", + "sub_state": "running", + "unit_file_state": "enabled", + "condition_result": "yes", + "managed_files": [ + { + "path": "/etc/foo.conf", + "src_rel": "etc/foo.conf", + "owner": "root", + "group": "root", + "mode": "0644", + "reason": "modified_conffile", + } + ], + "excluded": [], + "notes": [], + } + ], + "package_roles": [], + } + + bundle.mkdir(parents=True, exist_ok=True) + (bundle / "state.json").write_text(json.dumps(state, indent=2), encoding="utf-8") + + manifest(str(bundle), str(out), fqdn=fqdn) + + # Host playbook exists. + assert (out / "playbooks" / f"{fqdn}.yml").exists() + + # Role defaults are safe/host-agnostic in site mode. + foo_defaults = (out / "roles" / "foo" / "defaults" / "main.yml").read_text( + encoding="utf-8" + ) + assert "foo_packages: []" in foo_defaults + assert "foo_managed_files: []" in foo_defaults + assert "foo_manage_unit: false" in foo_defaults + + # Host vars contain host-specific state. + foo_hostvars = (out / "inventory" / "host_vars" / fqdn / "foo.yml").read_text( + encoding="utf-8" + ) + assert "foo_packages" in foo_hostvars + assert "foo_managed_files" in foo_hostvars + assert "foo_manage_unit: true" in foo_hostvars + assert "foo_systemd_state: started" in foo_hostvars + + # Non-templated raw config is stored per-host under .files. + assert ( + out / "inventory" / "host_vars" / fqdn / "foo" / ".files" / "etc" / "foo.conf" + ).exists()