fix: only install stratis if requested#624
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
💤 Files with no reviewable changes (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR makes Stratis package installation conditional: it defines Stratis package variables and a computed enable flag, removes Stratis from distribution default package lists, adds conditional install/service tasks, updates tests (including cleanup), and documents the ostree image note. ChangesConditional Stratis Management
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #624 +/- ##
==========================================
- Coverage 16.54% 10.16% -6.39%
==========================================
Files 2 9 +7
Lines 284 2056 +1772
Branches 79 0 -79
==========================================
+ Hits 47 209 +162
- Misses 237 1847 +1610
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![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 (3)
tasks/main-blivet.yml (1)
77-82: ⚡ Quick winUse FQCN for service module or add noqa comment.
The service module should use the fully qualified collection name
ansible.builtin.servicefor consistency with other tasks in this file (e.g.,ansible.builtin.get_urlat line 10,ansible.builtin.apt_repositoryat line 16). If there's a specific reason to use the short name, add a noqa comment like the mount tasks do at lines 194 and 211.♻️ Proposed fix
- name: Ensure stratisd is running and enabled - service: + ansible.builtin.service: name: stratisd state: started enabled: true when: __storage_manage_stratis🤖 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 `@tasks/main-blivet.yml` around lines 77 - 82, The task titled "Ensure stratisd is running and enabled" uses the short module name "service"; change it to the fully qualified collection name "ansible.builtin.service" (or if you intentionally prefer the short name, add a noqa comment like the mount tasks do) so it matches other tasks (e.g., ansible.builtin.get_url, ansible.builtin.apt_repository) and avoids lint warnings; update the task where the name/ module is declared to use ansible.builtin.service (or append the same noqa pattern used at the mount tasks).tests/tests_stratis.yml (2)
248-252: 💤 Low valueUse FQCN for service module.
For consistency with the coding guidelines examples, use the fully qualified collection name
ansible.builtin.serviceinstead of the short formservice.♻️ Proposed refactor for FQCN consistency
- name: Ensure stratisd is stopped and disabled - service: + ansible.builtin.service: name: stratisd state: stopped enabled: falseAs per coding guidelines: All examples consistently use FQCN like
ansible.builtin.package,ansible.builtin.command, etc.🤖 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/tests_stratis.yml` around lines 248 - 252, Replace the short module name "service" with the fully qualified collection name "ansible.builtin.service" in the task titled "Ensure stratisd is stopped and disabled": locate the task with name "Ensure stratisd is stopped and disabled" and change the module key from "service:" to "ansible.builtin.service:" while keeping the same parameters (name: stratisd, state: stopped, enabled: false).
247-257: 💤 Low valueConsider adding verification for cleanup success.
The cleanup
alwaysblock stops the service and removes packages, but there are no assertions to verify the cleanup succeeded. Consider adding verification tasks to ensure:
- The
stratisdservice is actually stopped and disabled- The Stratis packages are fully removed
📋 Example verification tasks
- name: Verify stratisd service is stopped ansible.builtin.service_facts: register: services_after_cleanup failed_when: services_after_cleanup.ansible_facts.services['stratisd.service'].state | default('inactive') != 'inactive' - name: Verify stratis packages are removed ansible.builtin.package_facts: register: packages_after_cleanup failed_when: (packages_after_cleanup.ansible_facts.packages.keys() | list) | intersect(_storage_stratis_packages) | length > 0As per coding guidelines: "Tests should verify both success and failure scenarios" and "Use assert module to verify expected state after role execution."
🤖 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/tests_stratis.yml` around lines 247 - 257, Add verification tasks after the existing "Ensure stratisd is stopped and disabled" and "Remove stratis packages" tasks to assert cleanup succeeded: use ansible.builtin.service_facts (or service module facts) and fail/ansible.builtin.assert to ensure 'stratisd' state is inactive/disabled, and use ansible.builtin.package_facts (or package_facts) and assert to ensure keys intersecting _storage_stratis_packages is empty; reference the existing task names to insert these checks into the same always block so the play fails if the service remains active or packages are still present.
🤖 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 `@README-ostree.md`:
- Around line 9-11: The README instruction is ambiguous: update the text to tell
users to edit the input package list files (not the script output) to remove
stratis packages; specifically instruct them to open the distribution-specific
package list files such as `.ostree/packages-runtime-RedHat-9.txt` and remove
the `stratisd` and `stratis-cli` lines (or any other distro-specific package
file), then re-run `.ostree/get_ostree_data.sh` to regenerate the ostree image
package list; include a short concrete example sentence showing which package
names to remove.
In `@vars/main.yml`:
- Around line 21-23: Rename the internal variable _storage_stratis_packages to
follow the internal naming convention by changing it to
__storage_stratis_packages and update every reference to that variable
throughout the repo (including the place that currently reads
_storage_stratis_packages in the blivet task) so the new name
__storage_stratis_packages is used consistently; ensure you only change the
variable name and its references, preserving value content and YAML structure.
---
Nitpick comments:
In `@tasks/main-blivet.yml`:
- Around line 77-82: The task titled "Ensure stratisd is running and enabled"
uses the short module name "service"; change it to the fully qualified
collection name "ansible.builtin.service" (or if you intentionally prefer the
short name, add a noqa comment like the mount tasks do) so it matches other
tasks (e.g., ansible.builtin.get_url, ansible.builtin.apt_repository) and avoids
lint warnings; update the task where the name/ module is declared to use
ansible.builtin.service (or append the same noqa pattern used at the mount
tasks).
In `@tests/tests_stratis.yml`:
- Around line 248-252: Replace the short module name "service" with the fully
qualified collection name "ansible.builtin.service" in the task titled "Ensure
stratisd is stopped and disabled": locate the task with name "Ensure stratisd is
stopped and disabled" and change the module key from "service:" to
"ansible.builtin.service:" while keeping the same parameters (name: stratisd,
state: stopped, enabled: false).
- Around line 247-257: Add verification tasks after the existing "Ensure
stratisd is stopped and disabled" and "Remove stratis packages" tasks to assert
cleanup succeeded: use ansible.builtin.service_facts (or service module facts)
and fail/ansible.builtin.assert to ensure 'stratisd' state is inactive/disabled,
and use ansible.builtin.package_facts (or package_facts) and assert to ensure
keys intersecting _storage_stratis_packages is empty; reference the existing
task names to insert these checks into the same always block so the play fails
if the service remains active or packages are still present.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 76954fe2-6ea8-45b2-b294-9c90784a3226
📒 Files selected for processing (9)
README-ostree.mdtasks/main-blivet.ymltests/tests_stratis.ymlvars/Fedora.ymlvars/OracleLinux_9.ymlvars/RedHat_10.ymlvars/RedHat_8.ymlvars/RedHat_9.ymlvars/main.yml
💤 Files with no reviewable changes (5)
- vars/OracleLinux_9.yml
- vars/Fedora.yml
- vars/RedHat_9.yml
- vars/RedHat_10.yml
- vars/RedHat_8.yml
richm
force-pushed
the
fix-stratis-only-if-needed
branch
from
038beef to
de52aa0
Compare
Contributor
Author
|
[citest] |
Cause: stratis packages are always installed, and the daemon is always started, even if the user does not use stratis. Consequence: The system has unnecessary packages and services. To maintain a good security posture, no services should be installed and running if not required. Fix: Change the code so that stratis packages and services are deployed only if the user is managing stratis. Result: The storage role does not install unnecessary packages. Signed-off-by: Rich Megginson <rmeggins@redhat.com>
richm
force-pushed
the
fix-stratis-only-if-needed
branch
from
de52aa0 to
379c359
Compare
Contributor
Author
|
[citest] |
vojtechtrefny
approved these changes
Jun 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cause: stratis packages are always installed, and the daemon is always started,
even if the user does not use stratis.
Consequence: The system has unnecessary packages and services.
To maintain a good security posture, no services should be installed
and running if not required.
Fix: Change the code so that stratis packages and services are deployed
only if the user is managing stratis.
Result: The storage role does not install unnecessary packages.
Signed-off-by: Rich Megginson rmeggins@redhat.com
Summary by CodeRabbit
New Features
Documentation
Tests
Chores