Skip to content

Various AI inspired code changes (And MOAR TESTS) - #2015

Merged
PromoFaux merged 5 commits into
developmentfrom
claude-review
Apr 1, 2026
Merged

Various AI inspired code changes (And MOAR TESTS)#2015
PromoFaux merged 5 commits into
developmentfrom
claude-review

Conversation

@PromoFaux

@PromoFaux PromoFaux commented Mar 29, 2026

Copy link
Copy Markdown
Member

Summary

A handful of code quality and robustness improvements identified during a review of the codebase following the BATS test suite migration.

  • Log error if crontab install failsstart_cron() was calling /usr/bin/crontab without checking the return code. A malformed crontab would silently fail, leaving gravity updates and the update checker permanently disabled with no indication in the logs.

  • Expand BATS test coverage — adds two previously untested areas:

    • ADDITIONAL_PACKAGES: a third shared container is started with ADDITIONAL_PACKAGES=wget and a test verifies the package is installed
    • TAIL_FTL_LOG=0: an inline container test verifies that setting TAIL_FTL_LOG=0 suppresses FTL log output and emits the expected notice (the default enabled path is already exercised implicitly by CONTAINER_DEFAULT)
  • Replace eval with a bash array in build.sh — the previous approach built a command string and called eval, which is harder to reason about and requires careful quoting. Replacing it with a proper bash array makes argument handling explicit and removes the fragile string substitution used to toggle --no-cache and the tag value.

Test plan

  • All existing BATS tests pass across all platforms
  • New ADDITIONAL_PACKAGES test passes
  • New TAIL_FTL_LOG=0 test passes

By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code and I have tested my changes.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits.

  • I have read the above and my PR is ready for review.

🤖 Generated with Claude Code

@PromoFaux
PromoFaux requested a review from a team as a code owner March 29, 2026 13:24
@yubiuser

Copy link
Copy Markdown
Member

Fix contradictory .editorconfig indent settings — indent_style = space requires indent_size to be a number; was incorrectly set to tab. Changed to 4 to match the existing tab_width = 4.

This is wrong. tabs is perfectly valid.

https://github.com/editorconfig/editorconfig/wiki/EditorConfig-Properties#indent_size

@yubiuser yubiuser left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace eval with bash array in build.sh — the previous approach built a command string and passed it to eval, which is vulnerable to shell injection if branch names or fork names contain metacharacters.

This is certainly correct, but what is the attack vector here? Someone tricking themselves into a shell injection?

Comment thread src/bash_functions.sh
@PromoFaux
PromoFaux force-pushed the claude-review branch 4 times, most recently from e541815 to b69a091 Compare March 29, 2026 14:10
@PromoFaux

Copy link
Copy Markdown
Member Author

(Note: this PR was largely written by Claude — responding here on its behalf)

Fair point on both counts.

The editorconfig change has already been dropped — that was simply incorrect on my part.

On the eval comment: you're right, "shell injection" was an overstatement for a local developer tool where the only person supplying input is the developer themselves. The commit message has been updated to describe it more honestly — it's a code quality preference (explicit array vs. opaque string + eval) rather than a security fix.

@yubiuser

Copy link
Copy Markdown
Member

I think we could improve the overall test logic and readability. Currently, we have 3 "default" container (setup in run.sh) we use in different test, but some test require custom container which we setup when needed (FTL shuts down cleanly, tail log is disabled). But we don't re-use functions from run.sh to start/remove custom contains in test_suite.bats. I suggest to not create any container in run.sh but use the functions from it to create all the containers we need inside test_suite.bats. First we could create the 'default' container, run the test for that, then remove it, then create the container with custom settings, run the appropiate test, then stop the container,... and so on.
This would not scatter various contains between those two files.

PromoFaux and others added 3 commits March 30, 2026 21:25
Previously a malformed or unreadable crontab would silently fail,
leaving gravity updates and the update checker permanently disabled
with no indication in the logs. Now logs a clear error message.

Signed-off-by: Adam Warner <me@adamwarner.co.uk>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add CONTAINER_PACKAGES shared container (ADDITIONAL_PACKAGES=wget)
  and a test that verifies the package is installed and on PATH
- Add inline container test for TAIL_FTL_LOG=true, verifying that
  FTL log lines tagged [FTL] appear in docker logs output

Signed-off-by: Adam Warner <me@adamwarner.co.uk>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Building a command string and calling eval is harder to reason about
and requires careful manual quoting. Replacing with a bash array makes
each argument explicit and removes the fragile string substitution
previously used to toggle --no-cache and update the tag value.

Signed-off-by: Adam Warner <me@adamwarner.co.uk>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Container creation, waiting, and teardown previously lived in run.sh
and were passed to test_suite.bats via exported environment variables.
This scattered container lifecycle between two files and required
custom inline logic for tests that needed their own container.

Replace test_suite.bats with two focused files, each owning their
container via setup_file/teardown_file:

- test_default.bats: plain container; covers FTL binary, cron,
  password generation, and graceful shutdown (run last, sequentially)
- test_env_vars.bats: custom UID/GID, FTLCONF_ vars, ADDITIONAL_PACKAGES,
  and TAIL_FTL_LOG=0 all exercised in a single container

Extract start_container and wait_for_log into test/helpers.sh, which
each .bats file loads directly. run.sh now only builds the image,
installs BATS, and invokes the test files.

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Adam Warner <me@adamwarner.co.uk>
@PromoFaux

Copy link
Copy Markdown
Member Author

@yubiuser - see latest commit, I actually pared it down quite a lot from what it was originally suggesting - it wanting 7 different BATS files with various container scenarios.

These two container scenarios cover all use cases we have in the tests currently and there is no need to start a fresh container for every single test.

Does that make it easier to follow?

Add tests for areas of container behaviour not previously exercised:

- crond is running (not just that the crontab file is valid)
- Logrotate config is installed at /etc/pihole/logrotate
- Default DNS upstreams (8.8.8.8/8.8.4.4) applied when none configured
- Web interface accessible at /admin/ (default port and custom port)
- /pihole.docker.tag build metadata file is present
- macvendor.db is present and configured in FTL
- FTL is running as the pihole user (validates DNSMASQ_USER default)
- Capabilities are applied to pihole-FTL (validates fix_capabilities)
- WEBPASSWORD_FILE reads the web password from a Docker secret

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Adam Warner <me@adamwarner.co.uk>
Comment thread test/run.sh
@PromoFaux PromoFaux changed the title Various code quality improvements Various AI inspired code changes (And MOAR TESTS) Apr 1, 2026
@PromoFaux
PromoFaux merged commit a856b71 into development Apr 1, 2026
15 checks passed
@PromoFaux
PromoFaux deleted the claude-review branch April 1, 2026 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants