[codex] tighten data rotation, cleanup, and service lifecycle defaults#72
[codex] tighten data rotation, cleanup, and service lifecycle defaults#72paudley wants to merge 6 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new data-cleanup command to the management CLI for removing stale pytest backup directories, alongside refactoring AppArmor profile loading into a dedicated library. It also includes integration tests for the cleanup logic and updates repository configurations and documentation. Feedback suggests improving the portability of the data-cleanup script by providing fallbacks for GNU-specific stat and du flags used on macOS and enhancing error resilience during file removal to prevent the script from crashing on permission errors.
| local modified | ||
| modified=$(stat -c '%Y' "${path}") | ||
| if ((modified <= cutoff)); then | ||
| local bytes | ||
| bytes=$(du -s -B1 "${path}" | awk '{print $1}') | ||
| candidates+=("${path}") | ||
| candidate_count=$((candidate_count + 1)) | ||
| total_bytes=$((total_bytes + bytes)) | ||
| fi |
There was a problem hiding this comment.
The use of GNU-specific flags for stat (-c) and du (-B) will cause this command to fail on macOS, which is a common development environment. Additionally, since these directories often contain database files with restricted permissions, these commands might fail even on Linux if the user lacks read access to all subdirectories. With set -e and pipefail enabled, any such failure will crash the entire script. Consider providing fallbacks for portability and handling errors to prevent the script from terminating prematurely.
| local modified | |
| modified=$(stat -c '%Y' "${path}") | |
| if ((modified <= cutoff)); then | |
| local bytes | |
| bytes=$(du -s -B1 "${path}" | awk '{print $1}') | |
| candidates+=("${path}") | |
| candidate_count=$((candidate_count + 1)) | |
| total_bytes=$((total_bytes + bytes)) | |
| fi | |
| while IFS= read -r -d '' path; do | |
| local modified | |
| # Attempt GNU stat, fallback to BSD/macOS stat | |
| if ! modified=$(stat -c '%Y' "${path}" 2>/dev/null); then | |
| modified=$(stat -f '%m' "${path}" 2>/dev/null || echo 0) | |
| fi | |
| if ((modified > 0 && modified <= cutoff)); then | |
| local bytes | |
| # Attempt GNU du with bytes, fallback to du -k (KB) and convert to bytes | |
| if ! bytes=$(du -s -B1 "${path}" 2>/dev/null | awk '{print $1}'); then | |
| bytes=$(du -sk "${path}" 2>/dev/null | awk '{print $1 * 1024}' || echo 0) | |
| fi | |
| candidates+=("${path}") | |
| candidate_count=$((candidate_count + 1)) | |
| total_bytes=$((total_bytes + bytes)) | |
| fi | |
| done < <(find "${backup_root}" -mindepth 1 -maxdepth 1 -type d -print0 | sort -z) |
| for candidate in "${candidates[@]}"; do | ||
| rm -rf -- "${candidate}" | ||
| done |
There was a problem hiding this comment.
If rm -rf fails for a specific candidate (e.g., due to permission issues), the script will terminate immediately because of set -e. It would be more resilient to log a warning and continue attempting to remove other stale candidates.
| for candidate in "${candidates[@]}"; do | |
| rm -rf -- "${candidate}" | |
| done | |
| for candidate in "${candidates[@]}"; do | |
| rm -rf -- "${candidate}" || echo "[data-cleanup] warning: failed to remove ${candidate}" >&2 | |
| done |
There was a problem hiding this comment.
Pull request overview
Adds an operator-facing manage.sh data-cleanup command to report/remove stale pytest data stashes under data/.pytest_backups, along with tests and documentation to codify the safety/retention contract. The PR also modularizes the AppArmor loader and updates repository hook configuration.
Changes:
- Add
data-cleanupcommand (dry-run by default, retention-based selection, optional guarded deletion with--execute/--force, optional--jsonoutput). - Extract AppArmor helper into
scripts/lib/apparmor.shand wire it intomanage.sh. - Add focused pytest coverage + module docs, and document the workflow in
README.md/docs.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
scripts/manage.sh |
Registers data-cleanup, sources new helpers, and moves AppArmor loader implementation to a lib. |
scripts/lib/data_cleanup.sh |
Implements stale .pytest_backups discovery, reporting, and deletion safety gate. |
scripts/lib/apparmor.sh |
New standalone AppArmor profile loader module. |
tests/test_data_cleanup.py |
New tests validating selection rules, execution behavior, and running-container refusal. |
tests/__init__.py |
Establishes test-suite package contract docstring. |
tests/MODULE.md |
Documents test-suite constraints and cleanup safety expectations. |
README.md |
Documents the new data-cleanup operator workflow and safety behavior. |
docs/SOURCE_DOCS.md |
Indexes required source/module docs per documentation policy. |
repo_config.yaml |
Narrows enabled hook groups and disables docstring coverage gate. |
Comments suppressed due to low confidence (1)
scripts/manage.sh:389
ensure_composeruns unconditionally before dispatching commands, which meansmanage.sh data-cleanuprequires adockerbinary even for a dry-run filesystem report. Sincedata-cleanupcan operate without Docker/Compose in dry-run mode, consider making the Docker check conditional per-command (or validatingCOMPOSE_BINinstead of hard-requiringdocker).
ensure_compose
COMMAND=${CORE_DATA_SELECTED_COMMAND:-help}
shift || true
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| while IFS= read -r -d '' path; do | ||
| local modified | ||
| modified=$(stat -c '%Y' "${path}") | ||
| if ((modified <= cutoff)); then | ||
| local bytes | ||
| bytes=$(du -s -B1 "${path}" | awk '{print $1}') |
| candidate_count=$((candidate_count + 1)) | ||
| total_bytes=$((total_bytes + bytes)) | ||
| fi | ||
| done < <(find "${backup_root}" -mindepth 1 -maxdepth 1 -type d -print0 | sort -z) |
paudley
changed the title
Align pgBackRest, Prometheus, daily maintenance, and logical backup retention with the seven-day operational policy. Add source PostgreSQL log pruning and document the updated defaults so generated config, CLI help, and README guidance agree. Also mirror create-db RabbitMQ provisioning during drop-db by deleting the matching vhost when RabbitMQ is enabled. Cover the retention defaults and RabbitMQ cleanup path in lightweight tests.
Ignore generated SQLite WAL and shared-memory files created beside the coding-ethos code-intel database, and remove the duplicate database ignore entry.
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR consolidates infrastructure defaults, introduces lifecycle management tooling for test data cleanup, integrates RabbitMQ provisioning into database operations, and updates Docker service configuration. Default retention shifts from 30 to 7 days across environment, services, and scripts. New data-cleanup command removes stale test backups with safety gating. RabbitMQ helpers coordinate with database lifecycle. ChangesOperational Infrastructure & Lifecycle Tooling
Test Coverage & Documentation
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Pylint (4.0.5)tests/__init__.pyTraceback (most recent call last): tests/test_data_cleanup.pyTraceback (most recent call last): tests/test_lightweight.pyTraceback (most recent call last): Comment |
paudley
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docker-compose.yml`:
- Around line 115-116: The docker-compose port defaults for PgBouncer are
inconsistent with scripts/manage.sh: change the default fallback values so
PGBOUNCER_HOST_PORT uses 6432 and PGBOUNCER_EXTRA_HOST_PORT uses 5432 (i.e.,
swap the default values in the compose file where ${PGBOUNCER_HOST_PORT:-5432}
and ${PGBOUNCER_EXTRA_HOST_PORT:-6432} appear) so the compose-exposed primary
port matches the CLI URL generation logic in scripts/manage.sh which falls back
to PGBOUNCER_PORT/6432; update both occurrences (the lines around the shown diff
and the similar block at lines ~492-493) to preserve the existing host-port
contract.
In `@scripts/create_env.sh`:
- Line 180: The script is passing the secret variable valkey_password on the
python3 command line (python3 - "${valkey_password}"
"${secret_dir}/valkey_exporter_passwords.json" <<'PY'), which exposes it to
process listings; change the invocation to read the password from a file or
stdin instead: write/read the password from a secure file under ${secret_dir}
(or pass it via an environment variable piped into python stdin) and update the
python3 call to accept the password from that file/stdin rather than the
command-line argument so valkey_password never appears in process args.
- Around line 180-189: The JSON key written to valkey_exporter_passwords.json
currently hardcodes port 6379; change the script to use the VALKEY_PORT variable
(with default 6379) when constructing the redis URI so the key matches the
exporter --redis.addr. Specifically, update the here-doc invocation around the
Python snippet in scripts/create_env.sh so the Python receives or reads the port
(e.g., pass "${VALKEY_PORT:-6379}" as an argument or interpolate it into the
argv) and write the key "redis://valkey:${PORT}" (where PORT is the resolved
VALKEY_PORT) instead of "redis://valkey:6379", keeping the rest of the JSON
writing and chmod logic unchanged.
In `@scripts/daily_maintenance.sh`:
- Around line 239-241: The pruning step currently runs find against
/var/lib/postgresql/data/log unconditionally which fails the script if that
directory is missing; update the block that uses PRUNE_SOURCE_LOGS,
RETENTION_DAYS, and retention_mtime to first check that the directory exists
(e.g. test -d "/var/lib/postgresql/data/log") before invoking the compose_exec
bash -lc "find ..." command so pruning is skipped silently when the log
directory is absent.
In `@scripts/lib/data_cleanup.sh`:
- Around line 64-70: The data_cleanup_json_escape function only handles
backslashes, quotes and newlines; update it to also escape tabs and carriage
returns and to encode other control characters (U+0000..U+001F) as JSON \uXXXX
sequences so output is valid JSON. Specifically, inside data_cleanup_json_escape
add replacements for $'\t' -> \\t and $'\r' -> \\r (operating on local value),
and implement a fallback that scans remaining bytes in value and replaces any
byte <= 0x1F with a \uXXXX escape (e.g., using a loop with printf and hex
formatting) so non-printable control characters are emitted as \\u00NN.
- Line 130: Replace the GNU-only stat and du invocations with portable
fallbacks: for the modified timestamp assignment (the code that sets the
modified variable from stat), use "stat -c '%Y' \"${path}\" || stat -f %m
\"${path}\"" to capture the file's mtime in a way that works on both Linux and
macOS; and for the directory size calculation (where du -s -B1 is used), fall
back to "du -s -B1 \"${path}\" || du -sk \"${path}\" | awk '{print $1 * 1024}'"
so you get bytes on GNU systems or convert kilobytes-to-bytes on macOS — update
the lines that set modified and the size_bytes (or equivalent) variables
accordingly.
- Line 133: The current bytes calculation using `du -s -B1 "${path}" | awk
'{print $1}'` is brittle across GNU vs BSD du semantics; update the bytes
computation to a portable approach by using `du -sk "${path}" | awk '{print
$1*1024}'` (or use `stat` where available) so the script assigns a consistent
byte count to the `bytes` variable for the given `path`; locate the line that
sets `bytes` and replace the `du -s -B1` invocation with the portable `du -sk`
conversion (or an equivalent `stat` fallback) to ensure correct units on
macOS/BSD and Linux.
In `@scripts/lib/rabbitmq.sh`:
- Around line 64-74: Check for the owner user before creating the vhost: move
the rabbitmqctl list_users check (the pipeline using awk "{print \$1}" and grep
-Fxq "${owner}") to run before the vhost creation, and if the owner is missing
print the existing warning message to stderr and exit with non-zero (exit 1) so
callers treat it as failure; only after confirming the owner exists should you
run rabbitmqctl add_vhost "${vhost}" (if needed) and rabbitmqctl set_permissions
-p "${vhost}" "${owner}" ".*" ".*" ".*"; also remove the stray trailing "' sh
\"${vhost}\" \"${owner}\"" at the end.
- Around line 43-50: The current invocation passes ${pass} on the host command
line; instead feed the password into the container over stdin and read it inside
the shell snippet to avoid argv exposure. Modify the rabbitmq_exec call so it
does not supply "${pass}" as an argument and change the inline script to read
the password from stdin (e.g. use read -r user; read -r pass) before calling
rabbitmqctl add_user "${user}" "${pass}"; ensure the host-side invocation writes
the user and password to rabbitmq_exec's stdin (via a here-doc or printf) rather
than supplying the password as a command-line argument.
In `@tests/test_lightweight.py`:
- Line 46: The test at tests/test_lightweight.py is brittle because it asserts
an exact whitespace layout for the CLI help string; change the assertion in the
test (the line currently checking "--retention <days> Retention in days
(default 7)") to a more robust check — e.g. verify stable tokens like
"--retention", "<days>", and "Retention in days (default 7)" appear in
result.stdout (or use a regex that allows flexible spacing such as matching
"--retention\s+<days>.*Retention in days") so help-formatting changes won't
break the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f65e46a5-776a-4698-b4b5-0e18c5a0b949
📒 Files selected for processing (24)
.env.example.gitignoreREADME.mddocker-compose.ymldocs/SOURCE_DOCS.mdmonitoring/grafana/provisioning/alerting/.gitkeepmonitoring/grafana/provisioning/plugins/.gitkeeppostgres/initdb/00-render-config.shrepo_config.yamlscripts/create_env.shscripts/daily_maintenance.shscripts/lib/apparmor.shscripts/lib/bootstrap_ci.shscripts/lib/ci_ports.shscripts/lib/data_cleanup.shscripts/lib/db.shscripts/lib/maintenance.shscripts/lib/rabbitmq.shscripts/logical_backup_runner.shscripts/manage.shtests/MODULE.mdtests/__init__.pytests/test_data_cleanup.pytests/test_lightweight.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Validate Docker Build
🧰 Additional context used
🪛 Ruff (0.15.15)
tests/test_data_cleanup.py
[error] 55-55: subprocess call: check for execution of untrusted input
(S603)
tests/test_lightweight.py
[error] 15-15: subprocess call: check for execution of untrusted input
(S603)
[error] 16-16: Starting a process with a partial executable path
(S607)
[error] 91-91: subprocess call: check for execution of untrusted input
(S603)
[error] 130-130: subprocess call: check for execution of untrusted input
(S603)
[error] 168-168: subprocess call: check for execution of untrusted input
(S603)
🔇 Additional comments (26)
repo_config.yaml (1)
1-8: LGTM!Also applies to: 11-12
docs/SOURCE_DOCS.md (1)
1-7: LGTM!tests/MODULE.md (1)
1-10: LGTM!tests/__init__.py (1)
1-2: LGTM!Also applies to: 4-13
scripts/create_env.sh (1)
7-9: LGTM!scripts/lib/bootstrap_ci.sh (1)
232-243: Align Valkey exporter password JSON withVALKEY_PORT
docker-compose.ymlconfigures the exporter with--redis.addr=redis://valkey:${VALKEY_PORT:-6379}, butscripts/lib/bootstrap_ci.shalways writesvalkey_exporter_passwords.jsonwith the keyredis://valkey:6379. If the exporter matches credentials by the fullredis.addrstring (as implied by using--redis.addr), changingVALKEY_PORTwould break authentication..env.example (1)
9-13: LGTM!Also applies to: 25-27, 37-43, 53-53, 58-58, 61-63, 66-67, 69-73, 83-83, 88-88, 92-92, 94-94, 106-106, 111-111, 113-115, 120-126, 131-131, 138-140, 142-144, 148-159, 161-163, 168-179, 189-189, 192-195, 256-257, 262-288, 294-294, 299-299
monitoring/grafana/provisioning/plugins/.gitkeep (1)
1-1: LGTM!.gitignore (1)
52-53: LGTM!Also applies to: 58-58
postgres/initdb/00-render-config.sh (1)
226-226: LGTM!scripts/daily_maintenance.sh (1)
39-50: LGTM!Also applies to: 377-380
scripts/lib/maintenance.sh (1)
125-125: LGTM!scripts/manage.sh (1)
46-47: LGTM!Also applies to: 54-55, 179-179, 281-281, 409-411
scripts/lib/apparmor.sh (1)
13-39: LGTM!scripts/lib/data_cleanup.sh (6)
1-15: LGTM!
16-30: LGTM!
32-54: LGTM!
56-62: LGTM!
141-154: LGTM!
156-187: LGTM!Also applies to: 189-194
tests/test_data_cleanup.py (6)
1-22: LGTM!
24-41: LGTM!
44-62: LGTM!
65-72: LGTM!
75-100: LGTM!
103-129: LGTM!Also applies to: 132-148
| ${POSTGRES_PORT:-5433} ${PGBOUNCER_HOST_PORT:-5432} | ||
| ${PGBOUNCER_EXTRA_HOST_PORT:-6432} ${VALKEY_HOST_PORT:-6379} |
There was a problem hiding this comment.
Keep PgBouncer default host-port fallbacks consistent with CLI URL generation.
Lines 115-116 and Lines 492-493 swap the default host-port roles (PGBOUNCER_HOST_PORT→5432, PGBOUNCER_EXTRA_HOST_PORT→6432), but scripts/manage.sh still derives client URLs with PGBOUNCER_HOST_PORT falling back to PGBOUNCER_PORT (6432). With unset env vars, CLI-generated connection info and compose-exposed primary port diverge.
Suggested alignment (preserve existing host-port contract)
- - "${PGBOUNCER_HOST_PORT:-5432}:${PGBOUNCER_PORT:-6432}"
- - "${PGBOUNCER_EXTRA_HOST_PORT:-6432}:${PGBOUNCER_PORT:-6432}"
+ - "${PGBOUNCER_HOST_PORT:-${PGBOUNCER_PORT:-6432}}:${PGBOUNCER_PORT:-6432}"
+ - "${PGBOUNCER_EXTRA_HOST_PORT:-5432}:${PGBOUNCER_PORT:-6432}"Also applies to: 492-493
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docker-compose.yml` around lines 115 - 116, The docker-compose port defaults
for PgBouncer are inconsistent with scripts/manage.sh: change the default
fallback values so PGBOUNCER_HOST_PORT uses 6432 and PGBOUNCER_EXTRA_HOST_PORT
uses 5432 (i.e., swap the default values in the compose file where
${PGBOUNCER_HOST_PORT:-5432} and ${PGBOUNCER_EXTRA_HOST_PORT:-6432} appear) so
the compose-exposed primary port matches the CLI URL generation logic in
scripts/manage.sh which falls back to PGBOUNCER_PORT/6432; update both
occurrences (the lines around the shown diff and the similar block at lines
~492-493) to preserve the existing host-port contract.
| printf '%s\n' "${valkey_password}" >"${valkey_secret_file}" | ||
| chmod 0600 "${valkey_secret_file}" || true | ||
| set_env_value VALKEY_PASSWORD_FILE "./secrets/valkey_password" | ||
| python3 - "${valkey_password}" "${secret_dir}/valkey_exporter_passwords.json" <<'PY' |
There was a problem hiding this comment.
Avoid passing secrets as command-line arguments.
The Valkey password is passed directly as a command-line argument to Python, which exposes it in process listings (ps, /proc). An attacker with access to process information could capture the password during the brief window the script executes.
🔒 Proposed fix to read password from file
-python3 - "${valkey_password}" "${secret_dir}/valkey_exporter_passwords.json" <<'PY'
+python3 - "${valkey_secret_file}" "${secret_dir}/valkey_exporter_passwords.json" <<'PY'
import json
import sys
-password, path = sys.argv[1], sys.argv[2]
+password_path, output_path = sys.argv[1], sys.argv[2]
+with open(password_path, encoding="utf-8") as handle:
+ password = handle.read().strip()
-with open(path, "w", encoding="utf-8") as handle:
+with open(output_path, "w", encoding="utf-8") as handle:
json.dump({"redis://valkey:6379": password}, handle)
handle.write("\n")
PY🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/create_env.sh` at line 180, The script is passing the secret variable
valkey_password on the python3 command line (python3 - "${valkey_password}"
"${secret_dir}/valkey_exporter_passwords.json" <<'PY'), which exposes it to
process listings; change the invocation to read the password from a file or
stdin instead: write/read the password from a secure file under ${secret_dir}
(or pass it via an environment variable piped into python stdin) and update the
python3 call to accept the password from that file/stdin rather than the
command-line argument so valkey_password never appears in process args.
| python3 - "${valkey_password}" "${secret_dir}/valkey_exporter_passwords.json" <<'PY' | ||
| import json | ||
| import sys | ||
|
|
||
| password, path = sys.argv[1], sys.argv[2] | ||
| with open(path, "w", encoding="utf-8") as handle: | ||
| json.dump({"redis://valkey:6379": password}, handle) | ||
| handle.write("\n") | ||
| PY | ||
| chmod 0600 "${secret_dir}/valkey_exporter_passwords.json" || true |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify valkey_exporter password-file key resolution behavior
rg -nP --type=yaml -A5 -B5 'redis\.password-file' docker-compose.yml
rg -nP 'VALKEY_PORT' .env.exampleRepository: paudley/core_data
Length of output: 491
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact code that generates the password JSON
echo "==== scripts/create_env.sh:180-220 ===="
nl -ba scripts/create_env.sh | sed -n '160,230p'
# Show the valkey-exporter service and how redis.password-file is used
echo "==== docker-compose.yml: relevant sections for valkey_exporter ===="
rg -n --context=3 'valkey_exporter|redis\.password-file|redis\.addr' docker-compose.yml
echo
rg -n --context=5 'valkey_exporter' docker-compose.yml
# Identify exporter image/tag to cross-check expected password-file format
echo
echo "==== docker-compose.yml: images for valkey_exporter service ===="
rg -n --context=2 'image:.*valkey_exporter|image:.*redis_exporter|image:' docker-compose.yml | head -n 80
# Show .env.example for VALKEY_PORT and any other related variables
echo
echo "==== .env.example: VALKEY_PORT ===="
rg -n 'VALKEY_PORT' .env.example || true
echo
sed -n '1,200p' .env.exampleRepository: paudley/core_data
Length of output: 162
🌐 Web query:
valkey_exporter redis.password-file JSON key redis:// exact match port
💡 Result:
To use a JSON password file with the redis_exporter (used for Valkey instances), you must specify the file path via the --redis.password-file command-line flag or the REDIS_PASSWORD_FILE environment variable [1][2][3]. The JSON file must contain a mapping where keys are the full connection strings (including the redis:// or rediss:// protocol and the port) and values are the corresponding passwords [4][1][3]. For an exact match, the connection string used as the JSON key must exactly match the value configured in the exporter's target address [1][3]. If you are connecting to a non-standard port, it must be explicitly included in both the exporter's target address and the JSON key [1][3][5]. Example JSON format: { "redis://localhost:6379": "password123", "redis://my-valkey-host:6380": "secure-password-for-port-6380" } Key requirements for the JSON file: 1. Exact Match: The key in the JSON file must be the full URI used to connect to the instance [1][3]. If your exporter targets redis://localhost:6379, the JSON key must be exactly redis://localhost:6379. 2. Protocol Inclusion: You must include the protocol (e.g., redis:// or rediss://) in the JSON key [1][3]. 3. Port Inclusion: If the target instance uses a non-default port, the port must be included in the JSON key (e.g., :6380) [1][3][5]. 4. Precedence: The password file is only used if the --redis.password (or REDIS_PASSWORD) flag is empty [1][3]. Note: The redis_exporter repository also supports URL normalization where valkey:// is treated as redis:// and valkeys:// as rediss:// [6], ensuring compatibility with Valkey instances.
Citations:
- 1: https://github.com/oliver006/redis_exporter
- 2: https://github.com/canonical/redis_exporter
- 3: https://github.com/oliver006/redis_exporter/blob/master/README.md
- 4: Exporter doesn't work with TLS and AUTH valkey-io/valkey-helm#104
- 5: https://github.com/oliver006/redis_exporter/blob/1e180f6e3faceef415684ed70e4438e98721e811/README.md
- 6: https://github.com/oliver006/redis_exporter/blob/master/AGENTS.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "==== scripts/create_env.sh:180-210 ===="
sed -n '165,215p' scripts/create_env.sh
echo
echo "==== docker-compose.yml: valkey_exporter / redis.addr / redis.password-file ===="
rg -n --context=3 'valkey_exporter' docker-compose.yml || true
rg -n --context=2 'redis\.addr=|redis\.password-file' docker-compose.yml || true
echo
echo "==== docker-compose.yml: command args for valkey_exporter (broader context) ===="
# Print the command block around the redis addr flag we already saw
rg -n --context=8 '--redis\.addr=' docker-compose.yml || true
echo
echo "==== .env.example: VALKEY_PORT ===="
rg -n 'VALKEY_PORT' .env.example || trueRepository: paudley/core_data
Length of output: 4745
🌐 Web query:
redis_exporter password-file JSON key exact match redis:// include port
💡 Result:
To use the --redis.password-file option in redis_exporter, you must provide a JSON file where the keys are the full Redis connection URIs, including the port [1][2]. The exporter requires an exact match between the target being scraped and the URI defined in the JSON file [1][3]. Format Requirements: 1. JSON Structure: The file must be a JSON object mapping the full URI to the password [2]. Example: { "redis://localhost:6379": "your_password", "redis://192.168.1.10:6380": "another_password" } 2. Exact Match: You must include the redis:// scheme and the port number (e.g.,:6379) in the JSON key [1][2]. If the target URI requested (e.g., via the /scrape endpoint) does not exactly match the key in your JSON file, the exporter will fail to find the password [1][2]. 3. Configuration: Ensure you start the exporter with the --redis.password-file flag pointing to your file, and ensure --redis.password is left empty (or unset), as the password-file only takes effect when --redis.password is empty [1]. It is important to always include the protocol (redis:// or rediss://) and the specific port in your JSON keys to ensure successful matching [1][4].
Citations:
- 1: https://github.com/oliver006/redis_exporter/blob/master/README.md
- 2: Add scrape password file support oliver006/redis_exporter#465
- 3: How to config multi instance with password oliver006/redis_exporter#715
- 4: https://github.com/canonical/redis_exporter
Hardcoded port breaks valkey_exporter password-file mapping when VALKEY_PORT changes.
scripts/create_env.sh writes a single JSON key redis://valkey:6379, but docker-compose.yml configures the exporter target as --redis.addr=redis://valkey:${VALKEY_PORT:-6379}. redis_exporter expects the password-file JSON key to exactly match the full redis URI (including the port), so using a different VALKEY_PORT will prevent the exporter from finding the password for that address.
🔧 Proposed fix to respect `VALKEY_PORT`
Generate the JSON key using the same port value used in --redis.addr (e.g., defaulting to 6379), instead of hardcoding 6379.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/create_env.sh` around lines 180 - 189, The JSON key written to
valkey_exporter_passwords.json currently hardcodes port 6379; change the script
to use the VALKEY_PORT variable (with default 6379) when constructing the redis
URI so the key matches the exporter --redis.addr. Specifically, update the
here-doc invocation around the Python snippet in scripts/create_env.sh so the
Python receives or reads the port (e.g., pass "${VALKEY_PORT:-6379}" as an
argument or interpolate it into the argv) and write the key
"redis://valkey:${PORT}" (where PORT is the resolved VALKEY_PORT) instead of
"redis://valkey:6379", keeping the rest of the JSON writing and chmod logic
unchanged.
| elif [[ ${PRUNE_SOURCE_LOGS} == true && ${RETENTION_DAYS} =~ ^[0-9]+$ && ${RETENTION_DAYS} -gt 0 ]]; then | ||
| retention_mtime=$((RETENTION_DAYS - 1)) | ||
| compose_exec bash -lc "find /var/lib/postgresql/data/log -maxdepth 1 -type f \\( -name 'postgresql-*.log' -o -name 'postgresql-*.csv' \\) -mtime +${retention_mtime} -delete" |
There was a problem hiding this comment.
Guard source-log pruning when the log directory is absent.
At Line 241, find runs unconditionally. If /var/lib/postgresql/data/log is missing, find exits non-zero and the whole script aborts due to set -e, turning optional pruning into a hard failure.
Suggested fix
elif [[ ${PRUNE_SOURCE_LOGS} == true && ${RETENTION_DAYS} =~ ^[0-9]+$ && ${RETENTION_DAYS} -gt 0 ]]; then
retention_mtime=$((RETENTION_DAYS - 1))
- compose_exec bash -lc "find /var/lib/postgresql/data/log -maxdepth 1 -type f \\( -name 'postgresql-*.log' -o -name 'postgresql-*.csv' \\) -mtime +${retention_mtime} -delete"
+ compose_exec bash -lc "if [[ -d /var/lib/postgresql/data/log ]]; then find /var/lib/postgresql/data/log -maxdepth 1 -type f \\( -name 'postgresql-*.log' -o -name 'postgresql-*.csv' \\) -mtime +${retention_mtime} -delete; fi"
fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/daily_maintenance.sh` around lines 239 - 241, The pruning step
currently runs find against /var/lib/postgresql/data/log unconditionally which
fails the script if that directory is missing; update the block that uses
PRUNE_SOURCE_LOGS, RETENTION_DAYS, and retention_mtime to first check that the
directory exists (e.g. test -d "/var/lib/postgresql/data/log") before invoking
the compose_exec bash -lc "find ..." command so pruning is skipped silently when
the log directory is absent.
| data_cleanup_json_escape() { | ||
| local value=$1 | ||
| value=${value//\\/\\\\} | ||
| value=${value//\"/\\\"} | ||
| value=${value//$'\n'/\\n} | ||
| printf '%s' "${value}" | ||
| } |
There was a problem hiding this comment.
Add escapes for tabs, carriage returns, and control characters.
The current implementation only escapes backslashes, quotes, and newlines. While this works for typical file paths, it may produce invalid JSON if paths contain tabs, carriage returns, or other control characters.
🔧 Proposed fix to handle additional escape sequences
data_cleanup_json_escape() {
local value=$1
value=${value//\\/\\\\}
value=${value//\"/\\\"}
+ value=${value//$'\t'/\\t}
+ value=${value//$'\r'/\\r}
+ value=${value//$'\f'/\\f}
value=${value//$'\n'/\\n}
printf '%s' "${value}"
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| data_cleanup_json_escape() { | |
| local value=$1 | |
| value=${value//\\/\\\\} | |
| value=${value//\"/\\\"} | |
| value=${value//$'\n'/\\n} | |
| printf '%s' "${value}" | |
| } | |
| data_cleanup_json_escape() { | |
| local value=$1 | |
| value=${value//\\/\\\\} | |
| value=${value//\"/\\\"} | |
| value=${value//$'\t'/\\t} | |
| value=${value//$'\r'/\\r} | |
| value=${value//$'\f'/\\f} | |
| value=${value//$'\n'/\\n} | |
| printf '%s' "${value}" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/lib/data_cleanup.sh` around lines 64 - 70, The
data_cleanup_json_escape function only handles backslashes, quotes and newlines;
update it to also escape tabs and carriage returns and to encode other control
characters (U+0000..U+001F) as JSON \uXXXX sequences so output is valid JSON.
Specifically, inside data_cleanup_json_escape add replacements for $'\t' -> \\t
and $'\r' -> \\r (operating on local value), and implement a fallback that scans
remaining bytes in value and replaces any byte <= 0x1F with a \uXXXX escape
(e.g., using a loop with printf and hex formatting) so non-printable control
characters are emitted as \\u00NN.
| local path | ||
| while IFS= read -r -d '' path; do | ||
| local modified | ||
| modified=$(stat -c '%Y' "${path}") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
# Search for platform detection patterns or stat usage
rg -nP '\bstat\s+-[cf]' --type=sh -C3
rg -nP '\buname\b.*Darwin|OSTYPE.*darwin' --type=sh -C3Repository: paudley/core_data
Length of output: 1084
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Show the relevant section of data_cleanup.sh around the reported lines
sed -n '110,170p' scripts/lib/data_cleanup.sh | nl -ba | sed -n '1,120p'
# Find other GNU-specific du/stat usages and macOS fallbacks
rg -n --fixed-string "du -s -B" --type=sh
rg -n --fixed-string "stat -f '%m'" --type=sh
rg -nP "\bstat\s+-c\s+['\"]?%Y['\"]?" --type=sh
rg -nP "stat\s+-c\s+['\"]?%Y['\"]?|stat\s+-f\s+['\"]?%m['\"]?" --type=shRepository: paudley/core_data
Length of output: 104
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Show relevant section with line numbers (no nl dependency)
sed -n '110,170p' scripts/lib/data_cleanup.sh | cat -n
# Search for GNU-specific du/stat usage and any macOS fallbacks
rg -n --fixed-string "du -s -B" --type=sh .
rg -n --fixed-string "stat -f '%m'" --type=sh .
rg -nP "\bstat\s+-c\s+['\"]?%Y['\"]?" --type=sh .
rg -nP "stat\s+-c\s+['\"]?%Y['\"]?|stat\s+-f\s+['\"]?%m['\"]?" --type=sh .Repository: paudley/core_data
Length of output: 2872
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Re-print the exact problematic block with line numbers
sed -n '120,150p' scripts/lib/data_cleanup.sh | cat -n
# Search repo for the same portability-sensitive commands
rg -n --fixed-strings "du -s -B1" --type=sh .
rg -nP "\bstat\s+-c\s+['\"]?%Y['\"]?" --type=sh .
rg -n --fixed-strings "stat -f '%m'" --type=sh .
rg -nP "stat\s+-c\s+['\"]?%Y['\"]?|stat\s+-f\s+['\"]?%m['\"]?" --type=sh .Repository: paudley/core_data
Length of output: 1712
Fix macOS portability for stat/du in scripts/lib/data_cleanup.sh.
stat -c '%Y'(line 130) is GNU/BSD-incompatible; the repo already handles this elsewhere viastat -c ... || stat -f %m ...inscripts/daily_maintenance.sh, butdata_cleanup.shdoesn’t.du -s -B1(line 133) is also GNU-specific and has no macOS-compatible fallback in the codebase.
Proposed fix (for `stat`)
local path
while IFS= read -r -d '' path; do
local modified
- modified=$(stat -c '%Y' "${path}")
+ if [[ "$(uname -s)" == "Darwin" ]]; then
+ modified=$(stat -f '%m' "${path}")
+ else
+ modified=$(stat -c '%Y' "${path}")
+ fi
if ((modified <= cutoff)); then🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/lib/data_cleanup.sh` at line 130, Replace the GNU-only stat and du
invocations with portable fallbacks: for the modified timestamp assignment (the
code that sets the modified variable from stat), use "stat -c '%Y' \"${path}\"
|| stat -f %m \"${path}\"" to capture the file's mtime in a way that works on
both Linux and macOS; and for the directory size calculation (where du -s -B1 is
used), fall back to "du -s -B1 \"${path}\" || du -sk \"${path}\" | awk '{print
$1 * 1024}'" so you get bytes on GNU systems or convert kilobytes-to-bytes on
macOS — update the lines that set modified and the size_bytes (or equivalent)
variables accordingly.
| modified=$(stat -c '%Y' "${path}") | ||
| if ((modified <= cutoff)); then | ||
| local bytes | ||
| bytes=$(du -s -B1 "${path}" | awk '{print $1}') |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
# Search for du usage and platform detection
rg -nP '\bdu\s+.*-[Bk]' --type=sh -C3Repository: paudley/core_data
Length of output: 566
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== du usage with -B/-b flags =="
rg -nP '\bdu\s+.*-(B|b)\b' . || true
echo
echo "== du usage with bytes/size flags =="
rg -nP '\bdu\s+-(s|k|K|-h|b|B)\b' . || true
echo
echo "== platform detection for Darwin/BSD =="
rg -nP 'Darwin|uname\s+-s|OSTYPE|sysctl\s+-n\s+hw\.model' --glob='*.sh' --glob='*.bash' --glob='*.zsh' --glob='*.inc' --glob='*.plugin' . || trueRepository: paudley/core_data
Length of output: 262
🌐 Web query:
macOS BSD du does not support -B flag
💡 Result:
The macOS du command is a BSD-based utility, which differs from the GNU Coreutils du commonly found on Linux [1][2]. The assertion that macOS du does not support the -B flag is incorrect; the flag is supported, but its function and behavior differ significantly from the GNU implementation [1][3]. In the BSD du version used on macOS, the -B blocksize option is used to calculate and display block counts based on a specified blocksize in bytes [4][1]. This is designed to estimate the space a file hierarchy would occupy on a filesystem with that specific block size [1][3]. This differs from GNU du, where -B (or --block-size) is primarily used to scale output units for display purposes [5][6]. Because the implementations are different, the two versions of du are not interchangeable in how they handle block size arguments [3]. If you are writing scripts intended to be portable between macOS and Linux, you should be aware that these flags do not perform the same task [3]. For simple output scaling on macOS, you may alternatively use the -k (KiB) or -m (MiB) flags, or set the BLOCKSIZE environment variable [1][7].
Citations:
- 1: https://man.freebsd.org/cgi/man.cgi?query=du&
- 2: https://www.unix.com/man_page/osx/1/du/
- 3: https://lists.freebsd.org/pipermail/svn-src-head/2008-November/000835.html
- 4: https://keith.github.io/xcode-man-pages/du.1.html
- 5: https://www.gnu.org/software/coreutils/manual/html_node/du-invocation.html
- 6: https://www.gnu.org/software/coreutils/du
- 7: https://ss64.com/mac/du.html
🌐 Web query:
macOS BSD du -B blocksize meaning output units with -s
💡 Result:
On macOS, the -B blocksize option for the du command instructs it to calculate and display file system usage in units of the specified blocksize in bytes [1][2]. When used in conjunction with the -s (summary) option, the output will represent the total size of the specified file or directory expressed in those custom-sized blocks [1][2]. Important details regarding this behavior include: 1. Functionality: The -B blocksize option performs a calculation that estimates how much space the examined file hierarchy would occupy if it were stored on a file system using that specific block size [1][2]. 2. Precedence and Interaction: This option is distinct from other output flags like -h (human-readable), -k (kilobytes), -m (megabytes), or -g (gigabytes), as well as the BLOCKSIZE environment variable [1][2]. 3. Defaults: If neither -B nor other specific size options are provided, and the BLOCKSIZE environment variable is not set, macOS du defaults to displaying output in 512-byte blocks [3][4][2]. Note that macOS uses the BSD version of du, which differs from the GNU coreutils version found on many Linux distributions [4][5][6]. While both share the -B option, their internal implementations and the way they handle block-size calculations can vary [1][2][5].
Citations:
- 1: https://man.freebsd.org/cgi/man.cgi?query=du&
- 2: https://man.freebsd.org/cgi/man.cgi?query=du&sektion=1&manpath=FreeBSD+8.4-RELEASE
- 3: https://www.unix.com/man_page/osx/1/du/
- 4: https://unix.stackexchange.com/questions/334865/du-s-and-du-hs-gives-different-results-on-os-x
- 5: https://www.gnu.org/software/coreutils/manual/html_node/du-invocation.html
- 6: https://manpages.debian.org/trixie/coreutils/du.1.en.html
Adjust du -s -B1 portability expectation on macOS/BSD
du -s -B1 is not GNU-only—macOS/BSD du supports -B—so the current command likely won’t fail due to a missing flag. The main risk is differing du semantics/units between GNU and BSD, which can make “bytes” derived from -B1 less reliable. Prefer a portable approach like -k and convert to bytes.
🔧 Proposed fix (portable `du` size)
- bytes=$(du -s -B1 "${path}" | awk '{print $1}')
+ bytes=$(du -sk "${path}" | awk '{print $1 * 1024}')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bytes=$(du -s -B1 "${path}" | awk '{print $1}') | |
| bytes=$(du -sk "${path}" | awk '{print $1 * 1024}') |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/lib/data_cleanup.sh` at line 133, The current bytes calculation using
`du -s -B1 "${path}" | awk '{print $1}'` is brittle across GNU vs BSD du
semantics; update the bytes computation to a portable approach by using `du -sk
"${path}" | awk '{print $1*1024}'` (or use `stat` where available) so the script
assigns a consistent byte count to the `bytes` variable for the given `path`;
locate the line that sets `bytes` and replace the `du -s -B1` invocation with
the portable `du -sk` conversion (or an equivalent `stat` fallback) to ensure
correct units on macOS/BSD and Linux.
| rabbitmq_exec sh -eu -c ' | ||
| user=$1 | ||
| pass=$2 | ||
| if rabbitmqctl list_users --silent | awk "{print \$1}" | grep -Fxq "${user}"; then | ||
| exit 0 | ||
| fi | ||
| rabbitmqctl add_user "${user}" "${pass}" | ||
| ' sh "${user}" "${pass}" |
There was a problem hiding this comment.
Don't pass application passwords through docker compose exec argv.
Line 50 puts ${pass} on the host-side command line, so the RabbitMQ credential is exposed to process inspection and any command logging wrapper around Docker. Feed the password over stdin (or another non-argv channel) instead.
Proposed fix
-rabbitmq_exec sh -eu -c '
-user=$1
-pass=$2
+printf '%s' "${pass}" | rabbitmq_exec sh -eu -c '
+user=$1
+pass=$(cat)
if rabbitmqctl list_users --silent | awk "{print \$1}" | grep -Fxq "${user}"; then
exit 0
fi
rabbitmqctl add_user "${user}" "${pass}"
-' sh "${user}" "${pass}"
+' sh "${user}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rabbitmq_exec sh -eu -c ' | |
| user=$1 | |
| pass=$2 | |
| if rabbitmqctl list_users --silent | awk "{print \$1}" | grep -Fxq "${user}"; then | |
| exit 0 | |
| fi | |
| rabbitmqctl add_user "${user}" "${pass}" | |
| ' sh "${user}" "${pass}" | |
| printf '%s' "${pass}" | rabbitmq_exec sh -eu -c ' | |
| user=$1 | |
| pass=$(cat) | |
| if rabbitmqctl list_users --silent | awk "{print \$1}" | grep -Fxq "${user}"; then | |
| exit 0 | |
| fi | |
| rabbitmqctl add_user "${user}" "${pass}" | |
| ' sh "${user}" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/lib/rabbitmq.sh` around lines 43 - 50, The current invocation passes
${pass} on the host command line; instead feed the password into the container
over stdin and read it inside the shell snippet to avoid argv exposure. Modify
the rabbitmq_exec call so it does not supply "${pass}" as an argument and change
the inline script to read the password from stdin (e.g. use read -r user; read
-r pass) before calling rabbitmqctl add_user "${user}" "${pass}"; ensure the
host-side invocation writes the user and password to rabbitmq_exec's stdin (via
a here-doc or printf) rather than supplying the password as a command-line
argument.
| vhost=$1 | ||
| owner=$2 | ||
| if ! rabbitmqctl list_vhosts --silent | grep -Fxq "${vhost}"; then | ||
| rabbitmqctl add_vhost "${vhost}" | ||
| fi | ||
| if ! rabbitmqctl list_users --silent | awk "{print \$1}" | grep -Fxq "${owner}"; then | ||
| echo "[rabbitmq] User ${owner} does not exist; created vhost ${vhost} without owner permissions." >&2 | ||
| exit 0 | ||
| fi | ||
| rabbitmqctl set_permissions -p "${vhost}" "${owner}" ".*" ".*" ".*" | ||
| ' sh "${vhost}" "${owner}" |
There was a problem hiding this comment.
Fail fast when the RabbitMQ owner is missing.
This path currently creates ${vhost}, prints a warning, and exits 0 when ${owner} does not exist. Because scripts/lib/db.sh Line 104 treats that as success, create-db can complete with a vhost that has no permissions, and a later create-user run will not repair that state. Check the owner first and return non-zero before creating the vhost.
Proposed fix
vhost=$1
owner=$2
-if ! rabbitmqctl list_vhosts --silent | grep -Fxq "${vhost}"; then
- rabbitmqctl add_vhost "${vhost}"
-fi
if ! rabbitmqctl list_users --silent | awk "{print \$1}" | grep -Fxq "${owner}"; then
- echo "[rabbitmq] User ${owner} does not exist; created vhost ${vhost} without owner permissions." >&2
- exit 0
+ echo "[rabbitmq] ERROR: User ${owner} does not exist; run create-user first." >&2
+ exit 1
+fi
+if ! rabbitmqctl list_vhosts --silent | grep -Fxq "${vhost}"; then
+ rabbitmqctl add_vhost "${vhost}"
fi
rabbitmqctl set_permissions -p "${vhost}" "${owner}" ".*" ".*" ".*"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| vhost=$1 | |
| owner=$2 | |
| if ! rabbitmqctl list_vhosts --silent | grep -Fxq "${vhost}"; then | |
| rabbitmqctl add_vhost "${vhost}" | |
| fi | |
| if ! rabbitmqctl list_users --silent | awk "{print \$1}" | grep -Fxq "${owner}"; then | |
| echo "[rabbitmq] User ${owner} does not exist; created vhost ${vhost} without owner permissions." >&2 | |
| exit 0 | |
| fi | |
| rabbitmqctl set_permissions -p "${vhost}" "${owner}" ".*" ".*" ".*" | |
| ' sh "${vhost}" "${owner}" | |
| vhost=$1 | |
| owner=$2 | |
| if ! rabbitmqctl list_users --silent | awk "{print \$1}" | grep -Fxq "${owner}"; then | |
| echo "[rabbitmq] ERROR: User ${owner} does not exist; run create-user first." >&2 | |
| exit 1 | |
| fi | |
| if ! rabbitmqctl list_vhosts --silent | grep -Fxq "${vhost}"; then | |
| rabbitmqctl add_vhost "${vhost}" | |
| fi | |
| rabbitmqctl set_permissions -p "${vhost}" "${owner}" ".*" ".*" ".*" | |
| ' sh "${vhost}" "${owner}" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/lib/rabbitmq.sh` around lines 64 - 74, Check for the owner user
before creating the vhost: move the rabbitmqctl list_users check (the pipeline
using awk "{print \$1}" and grep -Fxq "${owner}") to run before the vhost
creation, and if the owner is missing print the existing warning message to
stderr and exit with non-zero (exit 1) so callers treat it as failure; only
after confirming the owner exists should you run rabbitmqctl add_vhost
"${vhost}" (if needed) and rabbitmqctl set_permissions -p "${vhost}" "${owner}"
".*" ".*" ".*"; also remove the stray trailing "' sh \"${vhost}\" \"${owner}\""
at the end.
| ) | ||
| assert result.returncode == 0 | ||
| assert "core_data management CLI" in result.stdout | ||
| assert "--retention <days> Retention in days (default 7)" in result.stdout |
There was a problem hiding this comment.
Avoid brittle fixed-spacing assertion in CLI help test.
Line 46 asserts an exact whitespace layout; small help-formatting changes will fail the test without behavior regressions. Assert stable tokens instead.
Robust assertion example
- assert "--retention <days> Retention in days (default 7)" in result.stdout
+ assert "--retention <days>" in result.stdout
+ assert "Retention in days (default 7)" in result.stdout📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert "--retention <days> Retention in days (default 7)" in result.stdout | |
| assert "--retention <days>" in result.stdout | |
| assert "Retention in days (default 7)" in result.stdout |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_lightweight.py` at line 46, The test at tests/test_lightweight.py
is brittle because it asserts an exact whitespace layout for the CLI help
string; change the assertion in the test (the line currently checking
"--retention <days> Retention in days (default 7)") to a more robust check
— e.g. verify stable tokens like "--retention", "<days>", and "Retention in days
(default 7)" appear in result.stdout (or use a regex that allows flexible
spacing such as matching "--retention\s+<days>.*Retention in days") so
help-formatting changes won't break the test.
Summary
This PR publishes the full
data_rotationbranch. It tightens Core Data's local operational defaults around retention, generated runtime cleanup, monitoring, and service lifecycle behavior across the stack.What changed
./scripts/manage.sh data-cleanupfor staledata/.pytest_backups/inspection and deletion..pytest_backups, and guarded against running Compose containers unless--forceis used.DAILY_REMOVE_SOURCE_LOGS=truedestructive mode.create-userprovision matching RabbitMQ users andcreate-dbprovision matching RabbitMQ vhosts when RabbitMQ is enabled.drop-dbnow removes the matching RabbitMQ vhost when RabbitMQ is enabled..env.example, README guidance, and CLI help so operator-facing defaults match implemented behavior.Operator impact
PGBACKREST_RETENTION_FULL_TYPE=time.PROMETHEUS_RETENTION_TIME=7d.DAILY_RETENTION_DAYS, defaulting to seven days.drop-db.Validation
Passed:
env UV_CACHE_DIR=/tmp/uv-cache uv run python -m pytest tests/test_lightweight.py tests/test_data_cleanup.py- 9 passedcoding-ethos/.venv/bin/ruff --config ruff.toml check tests/test_lightweight.py- all checks passed./scripts/manage.sh config-checkinitially passed with rendered PostgreSQL config matching templatescoding-ethos/bin/coding-ethos-run policy-git commit ...hooks passed for the committed changesorigin/main..HEADreported good local signature status (G)Known validation limitations:
env UV_CACHE_DIR=/tmp/uv-cache uv run python -m pytest -k full_workflowfailed duringmanage.sh upbecause host port8080was already in use by the local environment, blocking the cAdvisor container bind. The fixture teardown removed the temporary containers and network../scripts/manage.sh config-checkreported livepg_hba.confnetwork allow-list drift after the compose workflow attempt. I left that runtime state untouched because it is host-specific generated configuration, not source for this branch.Rollback
Revert this PR to restore the previous cleanup command surface, retention defaults, daily-maintenance pruning behavior, and RabbitMQ lifecycle behavior. Operators can also override the new retention defaults explicitly in
.envwithout reverting code.