Add dnf Home Manager module for toolbox package management#3
Draft
Add dnf Home Manager module for toolbox package management#3
dnf Home Manager module for toolbox package management#3Conversation
Reviewer's GuideAdds a reusable Home Manager module for managing DNF packages inside Fedora toolbox containers, wires it into the flake and home configuration, and provides options for declarative installs, per-release installs, and optional upgrades with spinner-based UX. Sequence diagram for dnf activation in toolbox containerssequenceDiagram
participant User
participant HomeManager
participant DnfModule
participant HomeActivation
participant installScript as ShellScript
participant ToolboxEnv
participant dnf as Dnf
User->>HomeManager: apply configuration
HomeManager->>DnfModule: read dnf options
DnfModule->>HomeActivation: define dnfInstall entryAfter reloadSystemd
HomeManager->>HomeActivation: run dnfInstall script
HomeActivation->>installScript: execute installScript
installScript->>ToolboxEnv: check /run/.toolboxenv
alt not in toolbox
installScript-->>HomeActivation: exit
else in toolbox
installScript->>ToolboxEnv: read /etc/os-release VERSION_ID
installScript->>installScript: build INSTALL from install and releaseInstall
alt gum available
installScript->>dnf: gum spin dnf -y install INSTALL
else gum not available
installScript->>dnf: dnf -y install INSTALL
end
alt upgradeAll true
alt gum available
installScript->>dnf: gum spin dnf -y upgrade
else gum not available
installScript->>dnf: dnf -y upgrade
end
end
end
Class diagram for the new dnf Home Manager moduleclassDiagram
class DnfModule {
+bool enable
+list~string~ install
+attrs~list~string~~ releaseInstall
+bool upgradeAll
}
class HomeActivation {
+string dnfInstall
}
class Lib {
+mkEnableOption(description)
+mkOption(args)
+mkIf(condition)
+hm_dag_entryAfter(dependencies, script)
+concatStringsSep(sep, items)
+concatStrings(items)
+mapAttrsToList(fn, attrs)
+optionalString(condition, value)
}
class Config {
+DnfModule dnf
+HomeActivation home_activation
}
Config --> DnfModule : contains
Config --> HomeActivation : configures
DnfModule ..> Lib : uses
HomeActivation ..> DnfModule : derived from
HomeActivation ..> Lib : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Add a reusable Home Manager module that declaratively manages DNF packages inside Fedora toolbox containers. The module provides: - `dnf.install` — items to install via `dnf install` (packages, groups, paths, URLs, provides) - `dnf.releaseInstall` — per-Fedora-release items keyed by `VERSION_ID` - `dnf.upgradeAll` — optional full `dnf -y upgrade` on every activation The activation script uses `gum spin` for spinner output when available, falling back to plain echo. DNF's built-in idempotency handles already-installed items gracefully. Assisted-by: Claude Code Signed-off-by: Miroslav Vadkerti <mvadkert@redhat.com>
Collaborator
Author
|
@sourcery-ai review |
![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 1 issue, and left some high level feedback:
- The shell fragment in
installScriptbuildsINSTALLas a space-separated string and then expands it unquoted ($INSTALL), which can lead to word-splitting issues for items with spaces or special characters; consider using a bash array for the install list instead of a plain string. - Since this module is intended to run only inside toolbox, you might want to move the
/run/.toolboxenvcheck into a Nix-level condition (e.g. viamkIf) or at least add explicit checks/early exits for missingsudo/dnfbinaries to fail more clearly when used in other environments.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The shell fragment in `installScript` builds `INSTALL` as a space-separated string and then expands it unquoted (`$INSTALL`), which can lead to word-splitting issues for items with spaces or special characters; consider using a bash array for the install list instead of a plain string.
- Since this module is intended to run only inside toolbox, you might want to move the `/run/.toolboxenv` check into a Nix-level condition (e.g. via `mkIf`) or at least add explicit checks/early exits for missing `sudo`/`dnf` binaries to fail more clearly when used in other environments.
## Individual Comments
### Comment 1
<location path="modules/dnf.nix" line_range="33-41" />
<code_context>
+
+ # Install items (dnf handles already-installed items gracefully)
+ if [ -n "$INSTALL" ]; then
+ _dnf_run "DNF: installing $INSTALL" /usr/bin/sudo dnf -y install $INSTALL
+ fi
+
+ # Upgrade all packages
+ ${lib.optionalString cfg.upgradeAll ''
+ _dnf_run "DNF: upgrading all packages" /usr/bin/sudo dnf -y upgrade
+ ''}
+ '';
</code_context>
<issue_to_address>
**suggestion:** Hardcoding `/usr/bin/sudo` may break on images without sudo or with it in a different path.
In minimal or toolbox containers `sudo` may be missing or not at `/usr/bin/sudo`, causing `_dnf_run` to fail even though `dnf` is present. Consider either detecting `sudo` and falling back to direct `dnf` when absent, or just calling `sudo dnf ...` and relying on `$PATH` instead of a hardcoded path.
```suggestion
# Install items (dnf handles already-installed items gracefully)
if [ -n "$INSTALL" ]; then
if command -v sudo &>/dev/null; then
_dnf_run "DNF: installing $INSTALL" sudo dnf -y install $INSTALL
else
_dnf_run "DNF: installing $INSTALL" dnf -y install $INSTALL
fi
fi
# Upgrade all packages
${lib.optionalString cfg.upgradeAll ''
if command -v sudo &>/dev/null; then
_dnf_run "DNF: upgrading all packages" sudo dnf -y upgrade
else
_dnf_run "DNF: upgrading all packages" dnf -y upgrade
fi
''}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+33
to
+41
| # Install items (dnf handles already-installed items gracefully) | ||
| if [ -n "$INSTALL" ]; then | ||
| _dnf_run "DNF: installing $INSTALL" /usr/bin/sudo dnf -y install $INSTALL | ||
| fi | ||
|
|
||
| # Upgrade all packages | ||
| ${lib.optionalString cfg.upgradeAll '' | ||
| _dnf_run "DNF: upgrading all packages" /usr/bin/sudo dnf -y upgrade | ||
| ''} |
There was a problem hiding this comment.
suggestion: Hardcoding /usr/bin/sudo may break on images without sudo or with it in a different path.
In minimal or toolbox containers sudo may be missing or not at /usr/bin/sudo, causing _dnf_run to fail even though dnf is present. Consider either detecting sudo and falling back to direct dnf when absent, or just calling sudo dnf ... and relying on $PATH instead of a hardcoded path.
Suggested change
| # Install items (dnf handles already-installed items gracefully) | |
| if [ -n "$INSTALL" ]; then | |
| _dnf_run "DNF: installing $INSTALL" /usr/bin/sudo dnf -y install $INSTALL | |
| fi | |
| # Upgrade all packages | |
| ${lib.optionalString cfg.upgradeAll '' | |
| _dnf_run "DNF: upgrading all packages" /usr/bin/sudo dnf -y upgrade | |
| ''} | |
| # Install items (dnf handles already-installed items gracefully) | |
| if [ -n "$INSTALL" ]; then | |
| if command -v sudo &>/dev/null; then | |
| _dnf_run "DNF: installing $INSTALL" sudo dnf -y install $INSTALL | |
| else | |
| _dnf_run "DNF: installing $INSTALL" dnf -y install $INSTALL | |
| fi | |
| fi | |
| # Upgrade all packages | |
| ${lib.optionalString cfg.upgradeAll '' | |
| if command -v sudo &>/dev/null; then | |
| _dnf_run "DNF: upgrading all packages" sudo dnf -y upgrade | |
| else | |
| _dnf_run "DNF: upgrading all packages" dnf -y upgrade | |
| fi | |
| ''} |
Owner
There was a problem hiding this comment.
@sourcery-ai well, tbh if we want to make it right, we should rather check fi the user is not root, and if not require sudo ...
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.
Summary
modules/dnf.nix) that declaratively manages DNF packages inside Fedora toolbox containersdnf.installaccepts anythingdnf installsupports: package names,@groups, paths, URLs, providesdnf.releaseInstallallows per-Fedora-release items keyed byVERSION_IDdnf.upgradeAlloptionally runsdnf -y upgradeon every activationgum spinfor spinner output when available, falls back to plain echoAssisted-by: Claude Code
Summary by Sourcery
Add a reusable Home Manager module for managing DNF packages inside Fedora toolbox containers and wire it into the flake and home configuration.
New Features:
dnfHome Manager module that declaratively configures DNF package installation and upgrades inside toolbox containers.VERSION_ID.dnfconfiguration block inhome.nixfor enabling the module and specifying install behavior.Enhancements:
dnfmodule into the flake’shomeManagerModulesset and Home Manager module lists so it can be reused across configurations.