Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors package-manager configuration into a shared embedded YAML loader and adds pacman-based support so Arch-like distributions (including CachyOS) can be detected and installed correctly, addressing issue #20.
Changes:
- Add
pacmanpackage manager implementation and wire it into package-manager detection/selection (including fallback detection bypacmanbinary). - Introduce
pkgconfigembedded YAML loader shared by apt/dnf/pacman, and migrate package configuration + tests to it. - Adjust service manager detection to prefer
systemctlwhen available.
Reviewed changes
Copilot reviewed 20 out of 50 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/service/service.go | Prefer systemctl over service; simplify Load signature/flow |
| pkg/package_manager/yum.go | Reuse shared field keys when parsing yum output |
| pkg/package_manager/pkgconfig/config.go | New shared embedded YAML config loader (apt/dnf/pacman) |
| pkg/package_manager/pkgconfig/config_test.go | New tests for config merging/normalization and pacman configs |
| pkg/package_manager/pkgconfig/apt/README.md | Documentation for apt config hierarchy |
| pkg/package_manager/pkgconfig/apt/default.yaml | New apt default package mappings and install hooks |
| pkg/package_manager/pkgconfig/apt/default_arm64.yaml | New apt arm64 overrides |
| pkg/package_manager/pkgconfig/apt/debian.yaml | Debian-level apt overrides |
| pkg/package_manager/pkgconfig/apt/debian_6.yaml | Debian 6 overrides |
| pkg/package_manager/pkgconfig/apt/debian_7.yaml | Debian 7 overrides |
| pkg/package_manager/pkgconfig/apt/debian_8.yaml | Debian 8 overrides |
| pkg/package_manager/pkgconfig/apt/debian_9.yaml | Debian 9 overrides |
| pkg/package_manager/pkgconfig/apt/debian_10.yaml | Debian 10 overrides |
| pkg/package_manager/pkgconfig/apt/debian_11.yaml | Debian 11 overrides |
| pkg/package_manager/pkgconfig/apt/debian_12.yaml | Debian 12 overrides |
| pkg/package_manager/pkgconfig/apt/debian_13.yaml | Debian 13 overrides |
| pkg/package_manager/pkgconfig/apt/debian_sid.yaml | Debian sid overrides |
| pkg/package_manager/pkgconfig/apt/debian_duke.yaml | Debian codename override |
| pkg/package_manager/pkgconfig/apt/debian_forky.yaml | Debian codename override |
| pkg/package_manager/pkgconfig/apt/ubuntu.yaml | Ubuntu-level apt overrides |
| pkg/package_manager/pkgconfig/apt/ubuntu_12.04.yaml | Ubuntu 12.04 overrides |
| pkg/package_manager/pkgconfig/apt/ubuntu_14.04.yaml | Ubuntu 14.04 overrides |
| pkg/package_manager/pkgconfig/apt/ubuntu_16.04.yaml | Ubuntu 16.04 overrides |
| pkg/package_manager/pkgconfig/apt/ubuntu_18.04.yaml | Ubuntu 18.04 overrides |
| pkg/package_manager/pkgconfig/apt/ubuntu_20.04.yaml | Ubuntu 20.04 overrides |
| pkg/package_manager/pkgconfig/apt/ubuntu_22.04.yaml | Ubuntu 22.04 overrides |
| pkg/package_manager/pkgconfig/apt/ubuntu_22.10.yaml | Ubuntu 22.10 overrides |
| pkg/package_manager/pkgconfig/dnf/README.md | Documentation for dnf config hierarchy |
| pkg/package_manager/pkgconfig/dnf/default.yaml | New dnf default package mappings and install hooks |
| pkg/package_manager/pkgconfig/dnf/centos_7.yaml | CentOS 7-specific overrides |
| pkg/package_manager/pkgconfig/dnf/centos_8.yaml | CentOS 8-specific overrides |
| pkg/package_manager/pkgconfig/dnf/centos_10.yaml | CentOS 10-specific overrides |
| pkg/package_manager/pkgconfig/dnf/fedora.yaml | Fedora-level overrides |
| pkg/package_manager/pkgconfig/dnf/rocky.yaml | Rocky-level overrides |
| pkg/package_manager/pkgconfig/pacman/README.md | Documentation for pacman config hierarchy |
| pkg/package_manager/pkgconfig/pacman/default.yaml | New pacman default package mappings and hooks |
| pkg/package_manager/extended.go | New shared “extended” wrapper for YAML-driven steps across managers |
| pkg/package_manager/apt_strategy.go | Extract apt-specific install/remove behaviors into a strategy |
| pkg/package_manager/apt_search.go | Move apt list search into main package_manager package; add shared field constants |
| pkg/package_manager/apt.go | Switch to shared pkgconfig loader and shared extended wrapper |
| pkg/package_manager/dnf.go | Switch to shared pkgconfig loader and shared extended wrapper |
| pkg/package_manager/dnf_internal_test.go | Update internal tests to use shared extended wrapper |
| pkg/package_manager/pacman.go | New pacman package manager implementation + extended wrapper hookup |
| pkg/package_manager/pacman_internal_test.go | Tests for pacman info parsing + extended pacman creation |
| pkg/package_manager/manager.go | Add Arch routing + pacman binary fallback detection |
| pkg/package_manager/env_path_unix.go | Switch env PATH collection to use shared pkgconfig loader |
| pkg/package_manager/const.go | Add Arch distribution constant passthrough |
| pkg/package_manager/dnf/dnf.go | Removed old per-manager YAML loader (migrated to pkgconfig) |
| pkg/package_manager/dnf/dnf_test.go | Removed old dnf loader tests (replaced by pkgconfig tests) |
| pkg/package_manager/apt/apt_test.go | Removed old apt loader tests (replaced by pkgconfig tests) |
Comments suppressed due to low confidence (1)
pkg/package_manager/pkgconfig/config.go:63
buildCacheKeyusesosinf.Distribution.String()as-is, butLoadPackageslowercases the distribution for file selection (and tests expect case-insensitive behavior). If callers passDistribution: "CentOS"vs"centos", you’ll cache duplicate entries for the same effective config. Consider normalizing (e.g.,strings.ToLower) the distribution (and possibly codename) inside the cache key too, so cache behavior matches the loader’s case-insensitive semantics.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
102
to
114
| switch key { | ||
| case "PackageInfo": | ||
| info.Name = value | ||
| case "Architecture": | ||
| case fieldArchitecture: | ||
| info.Architecture = value | ||
| case "Version": | ||
| case fieldVersion: | ||
| info.Version = value | ||
| case "Size": | ||
| case fieldSize: | ||
| info.Size = value | ||
| case "Description": | ||
| case fieldDescription: | ||
| info.Description = value | ||
| case "Installed-Size": | ||
| size, err := strconv.Atoi(value) |
|
|
||
| cmd.Env = os.Environ() | ||
|
|
||
| log.Println('\n', cmd.String()) |
|
|
||
| cmd.Env = os.Environ() | ||
|
|
||
| log.Println('\n', cmd.String()) |
|
|
||
| cmd.Env = os.Environ() | ||
|
|
||
| log.Println('\n', cmd.String()) |
Coverage Report for CI Build 25974081021Coverage decreased (-0.2%) to 11.623%Details
Uncovered Changes
Coverage Regressions3 previously-covered lines in 2 files lost coverage.
Coverage Stats
💛 - Coveralls |
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
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.
Closes #20