Conversation
Redesign the ZFS configuration page with a two-column layout (settings on the left, disk list on the right), add a user-editable swap size field that defaults to actual RAM size, make the pool name always editable, and simplify pool type values to plain identifiers (stripe, mirror, raidz1/2/3). Consolidate duplicated next-button sensitivity logic into _update_next_button(), replace deprecated Gtk.STOCK icons with icon names, encrypt swap when GELI is enabled, and bump version to 0.4.
Reviewer's GuideRefactors the ZFS installation UI and behavior to a two-column layout with simplified pool-type handling and user-configurable swap size (defaulting to RAM), consolidates button-sensitivity logic, introduces encrypted swap when GELI is enabled, adds an initial_setup rc service, and bumps the installer version and data files accordingly. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
save_selection, consider validating the user-providedswap_size(e.g., non-negative and less than the availablesize) to avoid generating a negative or zerozfs_numand producing an invalid ZFS partition line. - In
get_ram_size_mb, it would be safer to avoidshell=Trueand to handle parsing errors (e.g., missing or malformedsysctloutput) by catching exceptions and returning a sensible default rather than raising.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `save_selection`, consider validating the user-provided `swap_size` (e.g., non-negative and less than the available `size`) to avoid generating a negative or zero `zfs_num` and producing an invalid ZFS partition line.
- In `get_ram_size_mb`, it would be safer to avoid `shell=True` and to handle parsing errors (e.g., missing or malformed `sysctl` output) by catching exceptions and returning a sensible default rather than raising.
## Individual Comments
### Comment 1
<location path="install_station/use_zfs.py" line_range="77-79" />
<code_context>
raise ValueError("Password cannot be empty when disk encryption is enabled")
-
+
size = int(cls.zfs_disk_list[0].partition('-')[2].rstrip()) - 512
- swap = 0
- zfs_num = size - swap
- if cls.disk_encrypt is True:
- dgeli = '.eli'
- else:
- dgeli = ''
+ swap_size = int(cls.swap_entry.get_text() or '0')
+ zfs_num = size - swap_size
+ dgeli = '.eli' if cls.disk_encrypt else ''
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against swap size larger than available disk size to avoid negative ZFS partition size.
Since `size` comes from the disk and `zfs_num = size - swap_size`, a user-provided `swap_size` larger than `size` will make `zfs_num` negative and likely produce an invalid partition layout. Please either clamp `swap_size` to `size` or raise a clear error when `swap_size >= size` before writing the config.
</issue_to_address>
### Comment 2
<location path="install_station/use_zfs.py" line_range="229-238" />
<code_context>
+ cls._update_next_button()
+
+ @classmethod
+ def on_password_changed(cls, _widget):
+ """
+ Handle password entry changes and update strength display.
+
+ Wraps the common password_strength function to extract the text
+ from the Entry widget and pass it with the strength label.
+
+ Args:
+ _widget: Entry widget that triggered the change (unused)
+ """
+ password_strength(cls.password.get_text(), cls.strenght_label)
+
+ @classmethod
</code_context>
<issue_to_address>
**issue (bug_risk):** Update the next button state when the primary password changes.
Since `_update_next_button` now owns the Next button sensitivity logic, `on_password_changed` should also call it so the button reflects the current match state. Otherwise, changing only the primary password can leave Next enabled even when the passwords no longer match.
</issue_to_address>
### Comment 3
<location path="install_station/system_calls.py" line_range="261-273" />
<code_context>
return dictionary
+def get_ram_size_mb() -> int:
+ """Query the system RAM size in megabytes using sysctl.
+
+ Returns:
+ System RAM size in MB
+ """
+ output = Popen(
+ 'sysctl -n hw.realmem',
+ shell=True,
+ stdout=PIPE,
+ universal_newlines=True
+ ).stdout.read().strip()
+ return int(output) // 1048576
+
+
</code_context>
<issue_to_address>
**suggestion:** Add basic error handling around the sysctl invocation and parsing.
This currently assumes `sysctl -n hw.realmem` always returns a valid integer. If the command fails or returns empty/non-numeric output, `int(output)` will raise and can break partition sizing. Please add minimal guarding (e.g., handle empty output, catch `ValueError`, and/or define a safe fallback).
```suggestion
def get_ram_size_mb() -> int:
"""Query the system RAM size in megabytes using sysctl.
Returns:
System RAM size in MB. Returns 0 if the RAM size cannot be determined.
"""
try:
process = Popen(
'sysctl -n hw.realmem',
shell=True,
stdout=PIPE,
universal_newlines=True,
)
except OSError:
# sysctl command not found or could not be executed
return 0
output = process.stdout.read().strip() if process.stdout is not None else ""
if not output:
# No output from sysctl; treat as unknown
return 0
try:
ram_bytes = int(output)
except ValueError:
# Non-numeric output from sysctl; treat as unknown
return 0
return ram_bytes // 1048576
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Replace the swap size text entry with a SpinButton capped to the selected disk size, consolidate _disk_count_valid and _update_next_button into a single _is_ready method that checks all preconditions, update the Next button from on_password_changed so it disables when passwords stop matching, re-enable the Next button in back_page so navigating back no longer leaves it stuck disabled, reduce the left panel gap, and fix a startup crash from mirror_selection firing before swap_entry was created.
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
2 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.
Redesign the ZFS configuration page with a two-column layout (settings
on the left, disk list on the right), add a user-editable swap size
field that defaults to actual RAM size, make the pool name always
editable, and simplify pool type values to plain identifiers (stripe,
mirror, raidz1/2/3). Consolidate duplicated next-button sensitivity
logic into _update_next_button(), replace deprecated Gtk.STOCK icons
with icon names, encrypt swap when GELI is enabled, and bump version
to 0.4.
Summary by Sourcery
Rework the ZFS installation UI and swap handling while wiring an initial-setup service into the installed system and updating packaging.
New Features:
Bug Fixes:
Enhancements:
Build: