-
Notifications
You must be signed in to change notification settings - Fork 61
fix: Allow running on systems without /etc/fstab present #562
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
When /etc/fstab is not present, we'll simply skip parsing it. Fixes: linux-system-roles#557 Resolves: RHEL-115033
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds a guard clause in parse() to skip parsing when /etc/fstab is missing, preventing errors on systems without the file. Class diagram for updated parse() method in blivet.pyclassDiagram
class Blivet {
+parse()
}
Blivet : +parse() // Now skips parsing if /etc/fstab is missing
Flow diagram for parse() method with /etc/fstab checkflowchart TD
A["Start parse()"] --> B{"Does /etc/fstab exist?"}
B -- No --> C["Return (skip parsing)"]
B -- Yes --> D["Open /etc/fstab"]
D --> E["Read lines and parse entries"]
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- Move the /etc/fstab existence check before resetting self._entries to avoid unnecessary state resets when the file is missing.
- Wrap the open call in try/except FileNotFoundError instead of using os.path.exists to better handle race conditions and permission errors.
- Return a consistent type (e.g. an empty list) from parse when /etc/fstab is absent instead of None to prevent downstream errors.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Move the /etc/fstab existence check before resetting self._entries to avoid unnecessary state resets when the file is missing.
- Wrap the open call in try/except FileNotFoundError instead of using os.path.exists to better handle race conditions and permission errors.
- Return a consistent type (e.g. an empty list) from parse when /etc/fstab is absent instead of None to prevent downstream errors.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #562 +/- ##
==========================================
- Coverage 16.54% 10.36% -6.19%
==========================================
Files 2 8 +6
Lines 284 2017 +1733
Branches 79 0 -79
==========================================
+ Hits 47 209 +162
- Misses 237 1808 +1571
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@vojtechtrefny what platforms are affected? |
|
The original reporter mentions running a live system over PXE, not sure which exactly, but there are more systems that don't use fstab by default, like Fedora CoreOS or bootc systems (and I guess it also possible to manually configure any system replace fstab with systemd mount units if someone really wants). |
When /etc/fstab is not present, we'll simply skip parsing it.
Fixes: #557
Resolves: RHEL-115033
Summary by Sourcery
Bug Fixes: