Skip to content

feat(gemini): An Ansible playbook to prune old log files and temporary directories, treating server maintenance as digital gardening.#3202

Open
polsala wants to merge 1 commit intomainfrom
ai/gemini-20260208-0829
Open

feat(gemini): An Ansible playbook to prune old log files and temporary directories, treating server maintenance as digital gardening.#3202
polsala wants to merge 1 commit intomainfrom
ai/gemini-20260208-0829

Conversation

@polsala
Copy link
Owner

@polsala polsala commented Feb 8, 2026

Implementation Summary

  • Utility: nightly-ansible-garden-trimmer
  • Provider: gemini
  • Location: ansible-playbooks/nightly-nightly-ansible-garden-trimm
  • Files Created: 5
  • Description: An Ansible playbook to prune old log files and temporary directories, treating server maintenance as digital gardening.

Rationale

  • Automated proposal from the Gemini generator delivering a fresh community utility.
  • This utility was generated using the gemini AI provider.

Why safe to merge

  • Utility is isolated to ansible-playbooks/nightly-nightly-ansible-garden-trimm.
  • README + tests ship together (see folder contents).
  • No secrets or credentials touched.
  • All changes are additive and self-contained.

Test Plan

  • Follow the instructions in the generated README at ansible-playbooks/nightly-nightly-ansible-garden-trimm/README.md
  • Run tests located in ansible-playbooks/nightly-nightly-ansible-garden-trimm/tests/

Links

  • Generated docs and examples committed alongside this change.

Mock Justification

  • Not applicable; generator did not introduce new mocks.

…y directories, treating server maintenance as digital gardening.
@polsala
Copy link
Owner Author

polsala commented Feb 8, 2026

🤖 Review by GROQ Agent

✅ What's solid

  • Clear purpose & naming – The playbook (trim_garden.yml) and its surrounding folder (nightly‑ansible‑garden‑trimm) are self‑describing, making it easy for a newcomer to understand the intent (“digital gardening”).
  • Idempotent directory handling – The file tasks that ensure log and temp directories exist (state: directory) are a good safety net for hosts where the paths may be missing.
  • Config‑driven – All mutable values live in vars/garden_config.yml and can be overridden with the extra var garden_config_path. This keeps the playbook reusable across environments.
  • Dry‑run support – The README explicitly shows the --check flag, encouraging safe experimentation before any destructive action.
  • Test harness – The Python test spins up a temporary filesystem, populates it with deterministic “old” and “new” files, runs the playbook via subprocess, and asserts the expected state. This end‑to‑end approach validates the real Ansible behaviour rather than just unit‑testing individual tasks.

🧪 Tests

  • Scope & reliability
    • The test covers the two core behaviours (log pruning & temp‑dir clearing) and checks that the temp directory itself is preserved – a solid functional coverage.
  • Potential flakiness
    • The test relies on the host’s ansible-playbook binary being on the PATH. In CI environments where Ansible isn’t pre‑installed, the test will abort with a FileNotFoundError.
      except FileNotFoundError:
          self.fail("Ansible command not found…")
      Action: Add an explicit CI step that installs Ansible (e.g., pip install ansible) or use ansible-test which ships with the repo’s tooling.
  • Time‑sensitive file ages
    • The test sets file timestamps using datetime.now() - timedelta(days=15). If the test runs across a daylight‑saving change or on a system with a non‑UTC clock, the age calculation could drift.
      Action: Use datetime.utcnow() consistently, or mock time.time() to a fixed epoch for deterministic age checks.
  • Subprocess invocation
    • Passing -e ansible_python_interpreter=… is unnecessary for a local run and can mask environment‑specific issues.
      Action: Remove that extra var unless you have a concrete need (e.g., testing against a non‑default interpreter).
  • Cleanup verification
    • The test asserts that the temporary directory is empty after the playbook runs, which is good. Consider also asserting that the debug messages appear in result.stdout to guarantee the reporting tasks are executed.

🔒 Security

  • Privilege escalation (become: yes)
    • The playbook elevates to root for both log and temp directories. While this is required for system‑wide paths like /var/log, it could be risky when the playbook is run against a host where the inventory includes non‑privileged users.
      Action:
      1. Scope become: yes to only the tasks that truly need it (e.g., wrap the log‑prune block in a block with become: yes).
      2. Add a become_user: root explicitly to avoid accidental escalation to another privileged account.
  • Path traversal
    • The file task that clears temp directories uses path: "{{ item }}/" with a trailing slash. If a user supplies a malicious path like ../../etc, Ansible will still act on it.
      Action: Validate temp_paths_to_clear in the vars file (or via a assert task) to ensure they are absolute and under an allowed prefix. Example:
      - name: Validate temp paths
        assert:
          that:
            - item.startswith('/tmp/') or item.startswith('/var/tmp/')
        loop: "{{ temp_paths_to_clear }}"
  • Variable injection
    • The playbook loads a user‑supplied garden_config_path via vars_files. If an attacker can control that path, they could load arbitrary YAML (including malicious Ansible modules).
      Action: Document that garden_config_path must be a trusted file and consider adding a sanity check:
      - name: Ensure config file is within the repo
        assert:
          that: garden_config_path is match('^.*/vars/.*\.yml$')

🧩 Docs/DX

  • README enhancements
    • Add a prerequisites section (e.g., “Ansible ≥ 2.12, Python 3.8+, sudo rights for log paths”).
    • Clarify the default location of the config file and the precedence order:
      1. garden_config_path extra var (overrides everything)
      2. ../vars/garden_config.yml relative to the playbook (current default)
    • Provide an example of running the playbook in check mode with a custom config:
      ansible-playbook -i src/inventory.ini src/trim_garden.yml \
        -e garden_config_path=custom_config.yml --check
  • Variable documentation
    • In vars/garden_config.yml, add comments describing each key, especially the expected format of log_paths_to_trim (absolute paths) and the unit of log_age_days.
  • Error handling guidance
    • Mention that if the playbook reports “failed” for a particular host, the most common cause is missing sudo rights. Suggest adding the user to the appropriate group or using become: true with a password‑less sudo rule.

🧱 Mocks/Fakes

  • Current approach – The test uses real filesystem objects, which is appropriate for an integration‑style test of an Ansible playbook.
  • Potential for faster unit tests
    • For CI speed, consider adding a lightweight unit‑test suite that mocks the ansible.builtin.find and ansible.builtin.file modules using ansible-test’s mock plugin. This can verify the task flow (e.g., that Delete old log files runs only when old_log_files.results is non‑empty) without spinning up a full subprocess.
  • Mock justification – The PR notes “Not applicable; generator did not introduce new mocks.” While true, you could still add a small mock‑based test to guard against regressions in task logic (e.g., accidental removal of the temp directory itself).

Takeaway: The utility is well‑structured, documented, and covered by an end‑to‑end test suite. Tightening the security checks around privilege escalation and path validation, making the test environment deterministic, and enriching the README will improve robustness and developer experience without altering the core functionality.

@polsala
Copy link
Owner Author

polsala commented Feb 8, 2026

🤖 Review by GROQ Agent

✅ What's solid

  • Clear purpose & naming – The “Digital Garden Trimmer” metaphor is well‑communicated and the playbook name (trim_garden.yml) matches the README description.
  • Idempotent directory handling – The file tasks that ensure target directories exist before any deletions are a good safety net.
  • Config‑driven design – All paths and thresholds live in vars/garden_config.yml (and can be overridden with garden_config_path), making the utility reusable across environments.
  • Separation of concerns – Inventory, vars, playbook, and tests are each in their own sub‑folder, keeping the repository tidy.
  • Dry‑run support – The README explicitly shows how to run the playbook with --check, which is essential for a maintenance utility.

🧪 Tests

  • Strengths

    • The test spins up a temporary directory hierarchy, creates both “old” and “new” files, and validates the exact post‑run state.
    • It exercises the full Ansible execution path (inventory, extra vars, config file), giving confidence that the playbook works end‑to‑end.
  • Actionable improvements

    • Make the test hermetic – Relying on the system‑wide ansible-playbook binary can cause flaky CI runs if the host has a different Ansible version or missing dependencies. Consider using ansible-runner or invoking the playbook via the Python API so the test can pin a specific version.
      from ansible_runner import run
      result = run(private_data_dir=self.temp_dir,
                   playbook='src/trim_garden.yml',
                   inventory=self.inventory_path,
                   extravars={'garden_config_path': self.config_path})
      self.assertEqual(result.rc, 0)
    • Add a check‑mode test – Verify that --check produces no file deletions while still reporting the intended actions. This guards against accidental side‑effects when users run a dry‑run.
    • Validate output messages – The playbook emits debug messages for deleted files and cleared directories. Asserting on result.stdout (or result.stderr) would ensure those messages stay in sync with the actual actions.
    • Parameterize age thresholds – The current test hard‑codes log_age_days: 7. Adding a small table‑driven test that runs the playbook with different thresholds (e.g., 0, 1, 30 days) would increase coverage of the find module’s age handling.
    • Clean up after failures – If the subprocess call raises, the temporary directory is still removed in tearDownClass, but an early return in the test could skip that. Using addCleanup or a try/finally block inside the test method guarantees cleanup even when self.fail() is triggered.

🔒 Security

  • Privilege escalation – The playbook uses become: yes for both log and temp directories. This is necessary for system paths like /var/log, but it also means the playbook runs with elevated rights on all hosts in the inventory.
    • Recommendation: Scope become to the specific tasks that truly need it, or add a become_user: root only where required. Example:
      - name: Ensure log dirs exist
        ansible.builtin.file:
          path: "{{ item }}"
          state: directory
          mode: '0755'
        loop: "{{ log_paths_to_trim }}"
        become: true
        become_user: root
  • Path injection via garden_config_path – Because the config file path is supplied as an extra variable, a malicious user could point it at an arbitrary location and cause the playbook to delete unintended files.
    • Mitigation: Validate the supplied path inside the playbook (or a pre‑task) to ensure it resides under a known safe directory, e.g.:
      - name: Validate garden_config_path
        assert:
          that:
            - garden_config_path is defined
            - garden_config_path.startswith(playbook_dir + '/vars/')
          fail_msg: "Config path must be inside the playbook's vars directory"
  • File mode choices – The temporary directories are created with mode 0777. While this mirrors typical /tmp permissions, it may be overly permissive on hardened hosts. Consider using 1777 (sticky bit) or a more restrictive mode if the environment permits.
  • Deletion semantics – The task that clears temporary directories uses state: absent on {{ item }}/. This removes all contents recursively, which is fine for a controlled temp dir but could be dangerous if a user mistakenly adds a production path to temp_paths_to_clear. Adding a safety check (e.g., ensure the path is under /tmp or /var/tmp) would reduce risk.

🧩 Docs / Developer Experience

  • README completeness – The guide covers installation, configuration, and execution. A few additions would make onboarding smoother:
    • Prerequisites section – Explicitly list required tools (ansible >= 2.9, Python 3.8+, ansible-playbook on PATH) and any OS‑level packages (e.g., sudo for privilege escalation).
    • Version pinning – Mention the tested Ansible version (e.g., “Developed and tested with Ansible 2.14”). This helps users avoid subtle incompatibilities.
    • Example inventory for remote hosts – Show how to target a remote host group, illustrating that the playbook is not limited to local execution.
    • Explain garden_config_path override – The README mentions the default location but does not demonstrate the extra‑var usage. Add a short snippet:
      ansible-playbook -i src/inventory.ini src/trim_garden.yml \
        -e "garden_config_path=./my_custom_config.yml"
  • Path consistency – The README’s “Run the playbook” command uses src/trim_garden.yml, which is correct relative to the repo root, but the earlier “Location” field in the PR body (ansible-playbooks/nightly-nightly-ansible-garden-trimm) contains a typo (trimm). Align the naming everywhere to avoid confusion.
  • Help output – Consider adding a --help style playbook entry point (e.g., a small wrapper script) that prints the available variables and defaults. This improves discoverability for new users.

🧱 Mocks / Fakes

  • Current approach – The test suite creates real files on disk and runs the actual Ansible binary. This provides high fidelity but couples the test to the host’s filesystem and to the presence of sudo (if the playbook attempts to become root).
  • Potential enhancements
    • Use ansible-test sandbox – Ansible ships with a testing harness that can execute playbooks in an isolated container. Leveraging it would eliminate the need for real sudo and guarantee a clean environment.
    • Mock the find module – If you want unit‑style tests that focus on the playbook logic rather than the underlying OS, you can replace the ansible.builtin.find task with a set_fact in a test‑specific playbook that returns a fabricated list of files. This speeds up CI and isolates logic errors.
    • Separate unit vs. integration tests – Keep the existing integration test (real filesystem) for end‑to‑end verification, but add a lightweight unit test that validates the Jinja2 loops and conditionals (e.g., that deleted_logs.results is correctly filtered). This can be done with ansible.utils or by rendering the playbook with ansible-playbook --list-tasks and inspecting the output.

Overall impression: The utility is well‑structured, documented, and includes a functional integration test. Addressing the security hardening points, tightening the test suite, and polishing the documentation will make the playbook safer and easier for contributors and operators to adopt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant