tests: replace hardcoded test pool names with unique checksummed names#995
tests: replace hardcoded test pool names with unique checksummed names#995tasleson wants to merge 1 commit into
Conversation
|
Warning Review limit reached
More reviews will be available in 50 minutes and 6 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
WalkthroughThis pull request introduces deterministic, collision-resistant test naming utilities by adding checksum-based name generation and validation functions. Hardcoded test pool and volume group identifiers across the test suite are replaced with generated names using shared constants, and the emergency cleanup script is updated to validate and target these dynamically-named resources. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/bin/snapm-test-name (1)
15-24: Reject invalidsubsystemandcontextvalues at generation time.This CLI currently accepts arbitrary strings, so it can emit names with spaces, slashes, or overlong segments that LVM/Stratis cannot actually create. Failing fast here keeps the new manual workflow reliable.
Suggested hardening
_generate() { local subsystem="$1" local context="$2" + [[ "$subsystem" =~ ^(lm|st)$ ]] || { + printf 'Invalid subsystem: %s\n' "$subsystem" >&2 + return 2 + } + [[ "$context" =~ ^[a-z0-9]+$ ]] || { + printf 'Invalid context: %s\n' "$context" >&2 + return 2 + } local random_hex random_hex=$(od -An -tx1 -N4 /dev/urandom | tr -d ' \n')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/bin/snapm-test-name` around lines 15 - 24, In _generate(), validate the incoming subsystem and context variables before building the name: ensure they are non-empty, match a safe charset (e.g., allow only [A-Za-z0-9._-] and reject spaces/slashes/other punctuation), and enforce a maximum segment length (pick a sane limit like 32 characters) to prevent overly long LVM/Stratis names; if validation fails, print a clear error to stderr and exit or return a non-zero status instead of emitting a name. Use the existing symbols subsystem, context, and the _generate function to locate where to add these checks and the error path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/bin/cleanup.sh`:
- Around line 25-39: The _validate_snapm_name() function currently only checks
the trailing separator and 4-char checksum; tighten it to validate the full
advertised format by matching the name against the pattern
snapm_<subsystem>_<context>_<8hex>_<4hex> (validate subsystem/context token
structure and that the two hex fields are the correct lengths and hex
characters), then compute/verify the trailing 4-char checksum as now; update the
validators in tests/_util.py and tests/bin/snapm-test-name to use the same
strict regex and checksum logic so the ownership check used before
vgremove/stratis destroy only accepts properly formatted test-owned names.
- Around line 41-49: The cleanup loop currently sets LVM_SYSTEM_DIR from the
caller's $PWD which breaks when the script is run outside the repo; instead
determine the repo root relative to the script and set LVM_SYSTEM_DIR from that.
In tests/bin/cleanup.sh compute the script directory using ${BASH_SOURCE[0]}
(e.g., resolve dirname "${BASH_SOURCE[0]}" and cd to its parent to get the repo
root) and export LVM_SYSTEM_DIR="$REPO_ROOT/tests/lvm" before calling vgremove
inside the for-loop (symbols to update: the LVM_SYSTEM_DIR export in the
vgremove block, and ensure this resolution happens prior to using
_validate_snapm_name / vgremove).
---
Nitpick comments:
In `@tests/bin/snapm-test-name`:
- Around line 15-24: In _generate(), validate the incoming subsystem and context
variables before building the name: ensure they are non-empty, match a safe
charset (e.g., allow only [A-Za-z0-9._-] and reject spaces/slashes/other
punctuation), and enforce a maximum segment length (pick a sane limit like 32
characters) to prevent overly long LVM/Stratis names; if validation fails, print
a clear error to stderr and exit or return a non-zero status instead of emitting
a name. Use the existing symbols subsystem, context, and the _generate function
to locate where to add these checks and the error path.
🪄 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: CHILL
Plan: Pro
Run ID: 2fb3ff91-40ed-4964-8cd6-6cfa9b3d0bb3
📒 Files selected for processing (8)
tests/_util.pytests/bin/cleanup.shtests/bin/snapm-test-nametests/test_boot.pytests/test_lvm2.pytests/test_manager.pytests/test_mounts.pytests/test_stratis.py
tasleson
force-pushed
the
test_asset_names
branch
from
789c9a1 to
d9ad1c2
Compare
Test resources previously used generic names (test_vg0, pool0, pool1) that could theoretically collide with real resources and made it impossible to verify ownership before cleanup. Replace with dynamically generated names in the format: snapm_<subsystem>_<context>_<8 random hex>_<4 hex SHA-256 checksum> The embedded checksum allows the cleanup script to validate that a resource was created by the test suite before destroying it. A standalone bash CLI tool (snapm-test-name) is provided for developers to manually generate and validate names. Resolves: snapshotmanager#978 Assisted-by: Claude <noreply@anthropic.com>
bmr-cymru
force-pushed
the
test_asset_names
branch
from
d9ad1c2 to
092d39a
Compare
|
Rebased to current main to pick up GitHub CI fix. |
2 participants
Test resources previously used generic names (test_vg0, pool0, pool1)
that could theoretically collide with real resources and made it
impossible to verify ownership before cleanup.
Replace with dynamically generated names in the format:
snapm_<8 random hex>_<4 hex SHA-256 checksum>
The embedded checksum allows the cleanup script to validate that a
resource was created by the test suite before destroying it. A
standalone bash CLI tool (snapm-test-name) is provided for developers
to manually generate and validate names.
Resolves: #978
Note: This may all seem like overkill, but this is one of those "Low-probability, high-consequence events"
Summary by CodeRabbit