add read only yum support (yum search, yum info)#10
Conversation
WalkthroughThe changes add support for the YUM package manager by updating documentation, CLI flags, parsing functions, implementing a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant SysPkg
participant YUM Manager
User->>CLI: Run syspkg command with --yum flag
CLI->>SysPkg: Parse flags, include YUM
SysPkg->>YUM Manager: IsAvailable()
YUM Manager-->>SysPkg: true/false
SysPkg->>YUM Manager: (e.g.) Find("nginx")
YUM Manager->>YUM Manager: Run "yum search nginx"
YUM Manager->>YUM Manager: Parse output
YUM Manager-->>SysPkg: []PackageInfo
SysPkg-->>CLI: Display results
CLI-->>User: Show package info
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @aijanai, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello team,
Gemini here, providing a summary of this pull request by @aijanai. This PR introduces initial support for the YUM package manager into the syspkg library. The implementation focuses on read-only operations, specifically search, list --installed, and info. Other package management actions like install, delete, and upgrade are explicitly marked as not yet implemented. The changes include the core YUM manager implementation, utility functions for parsing YUM command output, and corresponding tests. Additionally, the CLI flags for selecting specific package managers have been made visible in the main command-line application.
Highlights
- Read-Only Functionality: Implements
Find(search),ListInstalled, andGetPackageInfooperations for YUM. Other operations like install, delete, and upgrade return 'not implemented' errors. - YUM Output Parsing: Includes utility functions to parse the string output from
yum search,yum list --installed, andyum infocommands into structured package information. - YUM Tests: Adds unit tests to verify the functionality of the new YUM package manager implementation, including checks for implemented and unimplemented methods.
- CLI Visibility: Unhides the command-line flags (
--apt,--yum,--flatpak,--snap) in the main application, making it easier to select specific package managers.
Changelog
Click here to see the changelog
- README.md
- Updated the package manager compatibility table to reflect YUM's support for Search, List Installed, and Get Package Info.
- cmd/syspkg/main.go
- Changed the
Hiddenproperty for theapt,yum,flatpak, andsnapCLI flags fromtrue(or commented outtrue) tofalse.
- Changed the
- manager/yum/utils.go
- Added new file.
- Implemented
ParseFindOutputto parseyum searchresults. - Implemented
ParseListInstalledOutputto parseyum list --installedresults. - Implemented
ParsePackageInfoOutputto parseyum inforesults.
- manager/yum/yum.go
- Added new file.
- Implemented the
PackageManagerinterface for YUM. - Implemented
IsAvailable,GetPackageManager,Refresh,Find,ListInstalled, andGetPackageInfo. - Added placeholder implementations returning 'not implemented' errors for
Install,Delete,ListUpgradable,Upgrade,UpgradeAll, andAutoRemove.
- manager/yum/yum_test.go
- Added new file.
- Added tests for
IsAvailable. - Added tests to confirm unimplemented methods return errors.
- Added tests for
ListInstalled,GetPackageInfo, andFind.
- syspkg.go
- Imported the new
manager/yumpackage. - Added a
Yumboolean field to theIncludeOptionsstruct. - Included the YUM package manager in the
FindPackageManagersfunction based on theinclude.Yumoption.
- Imported the new
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
A new manager arrives,
For packages, it strives.
Though read-only it may be,
Search and info, you can see.
More features will come, believe!
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces read-only support for the YUM package manager, including search, list installed, and get package info functionalities. This is a valuable addition to syspkg.
The implementation includes parsing utilities for YUM command output, updates to the CLI to expose YUM, and new tests. The code is generally well-structured.
I've identified a few key areas for improvement:
- Robustness in Parsing: The output parsing functions in
manager/yum/utils.gocould be made more robust to prevent potential panics with unexpected input formats. - Efficiency: One of the parsing functions compiles a regular expression inside a loop, which could be optimized.
- Clarity of
Cleanoperation: TheCleanmethod's behavior might not align with typical expectations foryum clean.
Addressing these points will enhance the reliability and maintainability of the new YUM package manager implementation. Given the high-severity issues, I recommend addressing them before merging.
Summary of Findings
- Potential Panics in Output Parsing: The
ParseFindOutputandParseListInstalledOutputfunctions inmanager/yum/utils.gocould panic if theyumcommand output for package names doesn't strictly follow thename.archformat. This is due to direct indexing into the result ofstrings.Splitwithout checking its length. (Severity: High) - Regex Compilation in Loop:
ParseFindOutputcompiles a regular expression within a loop, which is inefficient for large inputs. (Severity: Medium) CleanMethod Behavior: TheCleanmethod inmanager/yum/yum.gocurrently performsyum clean expire-cache. Users might expect a more comprehensiveyum clean all. (Severity: Medium)
Merge Readiness
The pull request makes a good step towards YUM support. However, due to the high-severity issues identified (potential panics in parsing logic), I recommend that these are addressed before merging to ensure the stability of the YUM package manager implementation. The medium-severity issues should also be considered for improvement. I am unable to approve the pull request directly; please ensure these changes are reviewed and approved by other maintainers after the suggested modifications are made.
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 56199ea in 2 minutes and 25 seconds. Click for details.
- Reviewed
464lines of code in6files - Skipped
0files when reviewing. - Skipped posting
11draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. README.md:127
- Draft comment:
The table update for YUM clearly shows read-only support. Ensure the documentation details that install, remove, upgrade, and autoremove are not implemented. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
2. cmd/syspkg/main.go:293
- Draft comment:
Unhiding package manager flags (apt, yum, flatpak, snap) improves discoverability in the CLI help. This change looks good. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. manager/yum/utils.go:54
- Draft comment:
Potential index-out-of-range: When splitting parts[0] by '.', validate that the result has at least 2 elements before accessing indices 0 and 1. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The regex pattern explicitly requires a dot with content on both sides, so any line that passes the regex will have at least one dot and content on both sides. The parts[0] check ensures we have valid input. The regex validation makes the split safe. This seems like a false positive. The regex could theoretically be wrong or have bugs. Also, regex patterns can sometimes be tricky and not catch all edge cases. While regex can be tricky, this pattern is simple and clearly requires a dot with content on both sides. The code is structured to handle the package format which always has name.arch format. The comment should be deleted as the code already has sufficient validation through the regex pattern that ensures the dot and content exists.
4. manager/yum/utils.go:88
- Draft comment:
Ensure safe access when splitting the package identifier: check that splitting parts[0] by '.' in ParseListInstalledOutput returns at least two parts to avoid panics. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment identifies a real potential issue - accessing array indices without bounds checking could panic. However, yum list output format is very stable and standardized - packages always have architecture. The regex check in ParseFindOutput suggests the team is aware of format validation. The comment starts with "Ensure that" which is a red flag per the rules. I may be overconfident in the stability of yum output format. Even if rare, malformed input could crash the program. While the concern is valid, the comment's phrasing as a verification request rather than a direct code improvement suggestion makes it less actionable. The standardized format makes this a very low risk issue. Delete this comment since it starts with "Ensure that" and asks for verification rather than making a clear suggestion. If this was a real concern, it should be phrased as a direct code improvement.
5. manager/yum/utils.go:75
- Draft comment:
Redundant conversion: 'strings.Split(string(msg), "\n")' is unnecessary since msg is already a string. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
6. manager/yum/yum.go:59
- Draft comment:
Consider using a context with timeout for exec.Command calls (e.g., in Refresh) to prevent potential hangs in long-running or unresponsive commands. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
7. manager/yum/yum_test.go:11
- Draft comment:
These tests assume that yum is installed and certain packages (like 'nginx' and 'rpm') exist. Consider adding mocks or skipping tests in environments where yum is not available to avoid false negatives. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment raises a valid point about test dependencies, but the code already has an IsAvailable() check. The tests appear to be integration tests by design, meant to test against a real yum installation. Adding mocks would fundamentally change the test approach and may not be desirable. The comment is more of a suggestion than pointing out a clear issue. I could be wrong about the intention - maybe these really should be unit tests with mocks. Without seeing the implementation file, I can't be certain about the intended test strategy. Even with uncertainty about the intended strategy, the tests are clearly written as integration tests with IsAvailable() handling the yum dependency. The comment isn't pointing out a clear problem that needs fixing. Delete the comment as it's suggesting an alternative test approach rather than pointing out a clear issue that needs fixing. The existing IsAvailable() check already handles the main concern.
8. syspkg.go:45
- Draft comment:
IncludeOptions now includes 'Yum'. Ensure that the documentation and examples are updated to reflect this new, read-only support. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
9. manager/yum/yum.go:2
- Draft comment:
Typo: In the comment, "It provides an Go (golang)..." should be "It provides a Go (golang)...". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While the grammar correction is technically correct, I need to consider if this rises to the level of requiring a comment. Documentation clarity is important, but this is a very minor grammatical issue that doesn't affect code functionality or significantly impact readability. The rules state to not make comments that are obvious or unimportant. The grammar error could potentially confuse non-native English speakers. Documentation is a form of code and should be held to high standards. While documentation quality matters, this is such a minor grammatical issue that it doesn't significantly impact understanding. The meaning is perfectly clear either way. This comment, while technically correct, is too minor and doesn't meet the threshold for requiring a PR comment. It falls under the "do not make comments that are obvious or unimportant" rule.
10. manager/yum/yum.go:5
- Draft comment:
Typographical style issue: "Centos" should be capitalized as "CentOS" for consistency and correctness. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the comment is technically correct about the official capitalization of CentOS, this is just a documentation comment that provides background information. The capitalization doesn't affect functionality. According to the rules, we shouldn't make purely informative comments or comments about obvious/unimportant things. The proper capitalization could be important for professionalism and searchability of the documentation. Documentation standards might require proper product name capitalization. While proper capitalization is good practice, this is a minor stylistic issue in an informational comment that doesn't impact code functionality or understanding. The rules explicitly state not to make purely informative comments. Delete the comment as it's purely informative and doesn't affect code functionality or understanding.
11. manager/yum/yum_test.go:32
- Draft comment:
Typographical error: In the error message, "list-upgadeable" should be corrected to "list-upgradable". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While this is technically correct - there is a typo - it's an extremely minor issue in a test error message that would only be seen if the test fails. The typo doesn't affect functionality, test coverage, or code quality in any meaningful way. Test error messages are primarily for developer debugging. The typo could potentially cause confusion if someone is searching for error messages in the codebase. Also, maintaining consistent spelling throughout the codebase is generally good practice. While consistency is good, this is an extremely low-impact issue in a test error message. The meaning is still clear despite the typo, and it doesn't affect the actual functionality being tested. This comment should be deleted as it points out an extremely minor cosmetic issue that doesn't meaningfully impact code quality or functionality.
Workflow ID: wflow_mnFE2XCqbSb4RjOz
You can customize 
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (1)
cmd/syspkg/main.go (1)
365-368: 💡 Verification agent🧩 Analysis chain
Verify the YUM check in filterPackageManager.
This condition now includes the yum flag check, which is good. However, consider refactoring this long condition to be more maintainable.
🏁 Script executed:
#!/bin/bash # Check if yum is properly included in the CLI flag check grep -n "c.Bool(\"yum\")" cmd/syspkg/main.goLength of output: 206
Simplify the package manager flag check in filterPackageManager
Verified that
c.Bool("yum")is correctly included in the condition. The logic works, but the long inline&&chain is hard to maintain. Consider extracting the flag names into a slice and looping, for example:// cmd/syspkg/main.go:365–368 // before if !c.Bool("apt") && !c.Bool("flatpak") && !c.Bool("snap") && !c.Bool("yum") && !c.Bool("dnf") && !c.Bool("pacman") && !c.Bool("apk") && !c.Bool("zypper") { return availablePMs } // after packageManagers := []string{"apt", "flatpak", "snap", "yum", "dnf", "pacman", "apk", "zypper"} noneSelected := true for _, pm := range packageManagers { if c.Bool(pm) { noneSelected = false break } } if noneSelected { return availablePMs }
- Reduces duplication and makes adding/removing managers trivial.
- Improves readability and testability.
🧹 Nitpick comments (10)
manager/yum/utils.go (2)
29-29: Document unused parameteropts.The function ParseFindOutput has an
optsparameter that isn't used in the function body. Consider documenting why it's there or removing it if not needed.
45-45: Consider using compiled regex for better performance.Since the regular expression is used in a loop, compiling it once outside the loop would be more efficient.
+ // Compile regex outside the loop for better performance + packageLineRegex := regexp.MustCompile(`^[\w\d-]+\.[\w\d_]+`) + for _, line := range lines { if strings.HasPrefix(line, "=======") { continue } if strings.HasPrefix(line, "Last metadata") { continue } - if regexp.MustCompile(`^[\w\d-]+\.[\w\d_]+`).MatchString(line) { + if packageLineRegex.MatchString(line) {manager/yum/yum.go (2)
33-34: Consider adding documentation for ENV_NonInteractiveThe ENV_NonInteractive slice is empty. Consider adding documentation explaining why this is empty or what environment variables would be needed for non-interactive mode in YUM.
// ENV_NonInteractive contains environment variables used to set non-interactive mode for yum and dpkg. -var ENV_NonInteractive []string = []string{} +// For YUM, we don't need to set any environment variables for non-interactive mode. +var ENV_NonInteractive []string = []string{}
22-31: Add documentation for empty constantsSeveral constants are defined as empty strings, which might be confusing. Consider adding documentation explaining why these constants are empty or how they would be used.
// Constants used for yum commands const ( ArgsAssumeYes string = "-y" ArgsAssumeNo string = "--assumeno" ArgsQuiet string = "-q" + // YUM doesn't have an equivalent to the following APT arguments ArgsDryRun string = "" ArgsFixBroken string = "" ArgsPurge string = "" ArgsAutoRemove string = "" ArgsShowProgress string = "" )manager/yum/yum_test.go (6)
18-47: Rename TestYumPackageManagerNotAvailable to better reflect its purposeThe test name is misleading - it doesn't test if YUM is not available, but rather tests that certain methods are not implemented. A better name would be TestYumPackageManagerNotImplementedMethods.
-func TestYumPackageManagerNotAvailable(t *testing.T) { +func TestYumPackageManagerNotImplementedMethods(t *testing.T) {
11-16: Add skip mechanism for systems without YUMThis test assumes YUM is installed on the system, which might not be the case in all environments. Consider adding a mechanism to skip the test if YUM is not available.
func TestYumPackageManagerIsAvailable(t *testing.T) { yumManager := yum.PackageManager{} + // Skip test if yum is not available on the system + _, err := exec.LookPath("yum") + if err != nil { + t.Skip("Yum not available on this system, skipping test") + } if !yumManager.IsAvailable() { t.Fatal("YumPackageManager is not available") } }
49-59: Add skip mechanism for systems without YUM in ListPackages testThis test assumes YUM is installed on the system, which might not be the case in all environments. Consider adding a mechanism to skip the test if YUM is not available.
func TestYumPackageManagerListPackages(t *testing.T) { yumManager := yum.PackageManager{} + // Skip test if yum is not available on the system + if !yumManager.IsAvailable() { + t.Skip("Yum not available on this system, skipping test") + } opts:=manager.Options{} result,err:=yumManager.ListInstalled(&opts) if err!=nil{ t.Errorf("Should have been able to list correctly, %s",err) } if len(result)==0{ t.Fatal("Zero packages detected, there should have been at least one") } }
61-74: Add skip mechanism for systems without YUM in GetPackageInfo testThis test assumes YUM is installed and the "rpm" package exists, which might not be the case in all environments. Consider adding mechanisms to skip the test if YUM is not available or if the required package doesn't exist.
func TestYumPackageManagerGetPackageInfo(t *testing.T) { yumManager := yum.PackageManager{} + // Skip test if yum is not available on the system + if !yumManager.IsAvailable() { + t.Skip("Yum not available on this system, skipping test") + } opts:=manager.Options{} packages:="rpm" result,err:=yumManager.GetPackageInfo(packages, &opts) if err!=nil{ + // Skip if the package doesn't exist + if strings.Contains(err.Error(), "No matching packages") { + t.Skip("Package 'rpm' not found, skipping test") + } t.Errorf("Should have been able to get info correctly, %s",err) } if result.Name != packages { t.Errorf("rpm should be present, found %+v", result) } }
76-91: Add skip mechanism for systems without YUM in Find testThis test assumes YUM is installed and "nginx" is available in the repositories, which might not be the case in all environments. Consider adding mechanisms to skip the test if YUM is not available or if the required package isn't found.
func TestYumPackageManagerFind(t *testing.T) { yumManager := yum.PackageManager{} + // Skip test if yum is not available on the system + if !yumManager.IsAvailable() { + t.Skip("Yum not available on this system, skipping test") + } opts:=manager.Options{} packages:=[]string{"nginx"} result,err:=yumManager.Find(packages, &opts) if err!=nil{ + // Skip if search fails (e.g., no matching packages or repository issues) + t.Skip("Search for 'nginx' failed, skipping test:", err) + } if len(result)==0 { t.Errorf("nginx should be present, found %+v", result) } + // Check if any of the results match the package name + found := false + for _, pkg := range result { + if pkg.Name == packages[0] { + found = true + break + } + } + if !found { + t.Errorf("nginx should be available, found %+v", result) + } - if result[0].Name != packages[0] { - t.Errorf("nginx should be available, found %+v", result) - } }
32-32: Typo in error messageThere's a typo in the error message for the ListUpgradable test.
- t.Fatal("YumPackageManager should not support list-upgadeable") + t.Fatal("YumPackageManager should not support list-upgradable")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(1 hunks)cmd/syspkg/main.go(2 hunks)manager/yum/utils.go(1 hunks)manager/yum/yum.go(1 hunks)manager/yum/yum_test.go(1 hunks)syspkg.go(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
syspkg.go (2)
manager/yum/yum.go (1)
PackageManager(37-37)interface.go (1)
PackageManager(6-36)
manager/yum/yum_test.go (2)
manager/yum/yum.go (1)
PackageManager(37-37)manager/options.go (1)
Options(5-23)
manager/yum/utils.go (3)
manager/options.go (1)
Options(5-23)manager/yum/yum.go (1)
PackageManager(37-37)manager/packageinfo.go (1)
PackageStatusInstalled(10-10)
manager/yum/yum.go (3)
manager/options.go (1)
Options(5-23)syspkg.go (1)
PackageInfo(35-35)manager/yum/utils.go (3)
ParseFindOutput(29-64)ParseListInstalledOutput(69-104)ParsePackageInfoOutput(109-141)
🔇 Additional comments (5)
README.md (1)
127-127: LGTM! Accurate documentation of YUM support.The added row correctly indicates partial support for YUM with ✅ for implemented features (search, list installed, get package info) and ❓ for unimplemented ones.
syspkg.go (3)
28-28: LGTM! YUM package import added correctly.The yum package is properly imported.
45-45: LGTM! YUM option added to IncludeOptions.The Yum boolean field is properly added to the IncludeOptions struct.
80-80: LGTM! YUM package manager added to the manager list.YUM is correctly added to the list of package managers to be detected.
cmd/syspkg/main.go (1)
296-297: LGTM! Package manager flags visibility updated.The flags for apt, yum, flatpak, and snap package managers are now correctly visible in the CLI help, which is consistent with the available implementations.
Also applies to: 301-302, 326-327, 331-332
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
manager/yum/yum.go (5)
58-86:⚠️ Potential issueFix command argument splitting and add DryRun support
The command execution is incorrectly formatted and missing DryRun support.
-cmd := exec.Command(pm, "clean expire-cache") +cmd := exec.Command(pm, "clean", "expire-cache") cmd.Env = ENV_NonInteractive if opts == nil { opts = &manager.Options{ DryRun: false, Interactive: false, Verbose: false, } } +if opts.DryRun { + if opts.Verbose { + log.Println("Would run:", pm, "clean", "expire-cache") + } + return nil +}
88-100: 🛠️ Refactor suggestionAdd DryRun and Interactive support to Find method
The Find method should handle DryRun and Interactive options consistently with other methods in the package.
func (a *PackageManager) Find(keywords []string, opts *manager.Options) ([]manager.PackageInfo, error) { args := append([]string{"search"}, keywords...) cmd := exec.Command(pm, args...) cmd.Env = ENV_NonInteractive + if opts == nil { + opts = &manager.Options{ + DryRun: false, + Interactive: false, + Verbose: false, + } + } + + if opts.DryRun { + if opts.Verbose { + log.Println("Would run:", pm, append([]string{"search"}, keywords...)...) + } + return []manager.PackageInfo{}, nil + } + + if opts.Interactive { + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + cmd.Stdin = os.Stdin + err := cmd.Run() + if err != nil { + return nil, err + } + // In interactive mode, we can't parse the output since it went to stdout + return []manager.PackageInfo{}, nil + } + out, err := cmd.Output() if err != nil { return nil, err } return ParseFindOutput(string(out), opts), nil }
102-112: 🛠️ Refactor suggestionAdd DryRun and Interactive support to ListInstalled method
The ListInstalled method should handle DryRun and Interactive options consistently with other methods in the package.
func (a *PackageManager) ListInstalled(opts *manager.Options) ([]manager.PackageInfo, error) { args := []string{"list", "--installed"} cmd := exec.Command(pm, args...) cmd.Env = ENV_NonInteractive + + if opts == nil { + opts = &manager.Options{ + DryRun: false, + Interactive: false, + Verbose: false, + } + } + + if opts.DryRun { + if opts.Verbose { + log.Println("Would run:", pm, args) + } + return []manager.PackageInfo{}, nil + } + + if opts.Interactive { + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + cmd.Stdin = os.Stdin + err := cmd.Run() + if err != nil { + return nil, err + } + // In interactive mode, we can't parse the output since it went to stdout + return []manager.PackageInfo{}, nil + } + out, err := cmd.Output() if err != nil { return nil, err } return ParseListInstalledOutput(string(out), opts), nil }
123-125:⚠️ Potential issuePass options to Refresh in Clean method
The Clean method passes nil to Refresh instead of the options provided to Clean. This means that any options set by the caller of Clean will be lost.
func (a *PackageManager) Clean(opts *manager.Options) error { - return a.Refresh(nil) + return a.Refresh(opts) }
127-136: 🛠️ Refactor suggestionAdd DryRun and Interactive support to GetPackageInfo method
The GetPackageInfo method should handle DryRun and Interactive options consistently with other methods in the package.
func (a *PackageManager) GetPackageInfo(pkg string, opts *manager.Options) (manager.PackageInfo, error) { cmd := exec.Command(pm, "info", pkg) cmd.Env = ENV_NonInteractive + + if opts == nil { + opts = &manager.Options{ + DryRun: false, + Interactive: false, + Verbose: false, + } + } + + if opts.DryRun { + if opts.Verbose { + log.Println("Would run:", pm, "info", pkg) + } + return manager.PackageInfo{Name: pkg, PackageManager: pm}, nil + } + + if opts.Interactive { + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + cmd.Stdin = os.Stdin + err := cmd.Run() + if err != nil { + return manager.PackageInfo{}, err + } + // In interactive mode, we can't parse the output since it went to stdout + return manager.PackageInfo{Name: pkg, PackageManager: pm}, nil + } + out, err := cmd.Output() if err != nil { return manager.PackageInfo{}, err } return ParsePackageInfoOutput(string(out), opts), nil }
🧹 Nitpick comments (2)
manager/yum/yum.go (2)
19-31: Consider adding appropriate values for empty constantsSeveral constants are defined but have empty string values (ArgsDryRun, ArgsFixBroken, ArgsPurge, ArgsAutoRemove, ArgsShowProgress). Either add appropriate values for these constants or remove them if they're not applicable to YUM.
33-34: Environment variables for non-interactive mode are emptyThe ENV_NonInteractive slice is empty, which is different from other package managers that typically set environment variables like DEBIAN_FRONTEND=noninteractive for APT. Consider investigating if YUM has equivalent environment variables for non-interactive mode.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
README.md(1 hunks)cmd/syspkg/main.go(2 hunks)manager/yum/utils.go(1 hunks)manager/yum/yum.go(1 hunks)manager/yum/yum_test.go(1 hunks)osinfo/osinfo.go(1 hunks)syspkg.go(3 hunks)syspkg_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- README.md
- syspkg.go
- cmd/syspkg/main.go
- manager/yum/utils.go
- manager/yum/yum_test.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
syspkg_test.go (1)
osinfo/osinfo.go (1)
OSInfo(25-30)
manager/yum/yum.go (2)
manager/options.go (1)
Options(5-23)manager/yum/utils.go (3)
ParseFindOutput(29-69)ParseListInstalledOutput(74-112)ParsePackageInfoOutput(117-149)
🔇 Additional comments (6)
syspkg_test.go (1)
63-63: Good addition for Rocky Linux detectionAdding "rocky" alongside "rockylinux" ensures more robust detection of Rocky Linux distributions, which is necessary for proper YUM package manager detection. This matches real-world scenarios where the distribution ID in
/etc/os-releasemight be identified either way.osinfo/osinfo.go (2)
92-92: Improved distribution ID parsingThe added quote trimming for the distribution ID matches how the version ID is handled, ensuring consistent parsing regardless of whether quotes are present in the
/etc/os-releasefile. This is a good improvement for robustness.
94-95: Improved version string parsingSplitting the version string trimming into two separate steps - first removing the prefix, then trimming quotes - makes the code more readable and matches how the distribution ID is now processed. This creates consistency in how both fields are handled.
manager/yum/yum.go (3)
50-56: Appropriate stubs for unimplemented methodsThese stubs align with the PR's focus on read-only YUM support. The error messages clearly indicate that the functionality is not implemented.
138-140: Appropriate stub for unimplemented AutoRemove methodThis stub aligns with the PR's focus on read-only YUM support. The error message clearly indicates that the functionality is not implemented.
1-18: Comprehensive package documentationThe package documentation provides clear context about YUM, its relationship to DNF, and the purpose of this package. This is helpful for developers who will be working with this code.
aijanai
force-pushed
the
yum
branch
2 times, most recently
from
89fe029 to
3d81957
Compare
|
@bluet what do I need to do to unblock? make all the bots happy? |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
README.md(1 hunks)cmd/syspkg/main.go(2 hunks)manager/yum/utils.go(1 hunks)manager/yum/yum.go(1 hunks)manager/yum/yum_test.go(1 hunks)osinfo/osinfo.go(1 hunks)syspkg.go(4 hunks)syspkg_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- cmd/syspkg/main.go
🚧 Files skipped from review as they are similar to previous changes (6)
- osinfo/osinfo.go
- syspkg_test.go
- README.md
- manager/yum/yum_test.go
- manager/yum/utils.go
- manager/yum/yum.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
syspkg.go (5)
manager/yum/yum.go (1)
PackageManager(37-37)manager/apt/apt.go (1)
PackageManager(46-46)manager/flatpak/flatpak.go (1)
PackageManager(47-47)manager/snap/snap.go (1)
PackageManager(44-44)interface.go (1)
PackageManager(6-36)
🔇 Additional comments (3)
syspkg.go (3)
28-28: LGTM! Import follows established pattern.The yum package import is consistent with other package manager imports and properly integrates the new yum support.
45-45: LGTM! Struct field addition is consistent.The
Yum boolfield follows the same naming convention and pattern as other package manager fields in theIncludeOptionsstruct.
80-80: LGTM! Manager integration follows established pattern.The yum manager is properly integrated into the manager list using the same structure and pattern as other package managers.
according to the documentation, when GetPackageManager() is invoked with an empty name, "If the name is empty, the first available package manager will be returned.". But that was unimplemented
|
@aijanai thanks for the PR 😃 Can you resolve the conversations/change requests? If the bots are wrong, replying with your reason/thoughts would be fine, too. I'll test it on Ubuntu when they're resolved. If you've already tested on Ubuntu, please let me know. 👍 Sorry for being busy recently and might not check issues frequently. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
manager/yum/yum.go (4)
124-124: 🛠️ Refactor suggestionPass options to Refresh in Clean method.
The Clean method passes nil to Refresh instead of the options provided to Clean.
- return a.Refresh(nil) + return a.Refresh(opts)
127-136: 🛠️ Refactor suggestionAdd Interactive support to GetPackageInfo method.
The GetPackageInfo method doesn't handle the Interactive option. For consistency with other methods, it should support interactive mode.
func (a *PackageManager) GetPackageInfo(pkg string, opts *manager.Options) (manager.PackageInfo, error) { cmd := exec.Command(pm, "info", pkg) cmd.Env = ENV_NonInteractive + + if opts == nil { + opts = &manager.Options{ + DryRun: false, + Interactive: false, + Verbose: false, + } + } + + if opts.Interactive { + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + cmd.Stdin = os.Stdin + err := cmd.Run() + if err != nil { + return manager.PackageInfo{}, err + } + // In interactive mode, we can't parse the output since it went to stdout + return manager.PackageInfo{Name: pkg, PackageManager: pm}, nil + } + out, err := cmd.Output() if err != nil { return manager.PackageInfo{}, err } return ParsePackageInfoOutput(string(out), opts), nil }
102-112: 🛠️ Refactor suggestionAdd Interactive support to ListInstalled method.
The ListInstalled method doesn't handle the Interactive option. For consistency with other methods, it should support interactive mode.
func (a *PackageManager) ListInstalled(opts *manager.Options) ([]manager.PackageInfo, error) { args := []string{"list", "--installed"} cmd := exec.Command(pm, args...) cmd.Env = ENV_NonInteractive + + if opts == nil { + opts = &manager.Options{ + DryRun: false, + Interactive: false, + Verbose: false, + } + } + + if opts.Interactive { + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + cmd.Stdin = os.Stdin + err := cmd.Run() + if err != nil { + return nil, err + } + // In interactive mode, we can't parse the output since it went to stdout + return []manager.PackageInfo{}, nil + } + out, err := cmd.Output() if err != nil { return nil, err } return ParseListInstalledOutput(string(out), opts), nil }
88-100: 🛠️ Refactor suggestionAdd Interactive support to Find method.
The Find method doesn't handle the Interactive option. While DryRun isn't needed for read-only operations (as per previous feedback), Interactive mode should still be supported for consistency.
func (a *PackageManager) Find(keywords []string, opts *manager.Options) ([]manager.PackageInfo, error) { args := append([]string{"search"}, keywords...) cmd := exec.Command(pm, args...) cmd.Env = ENV_NonInteractive + if opts == nil { + opts = &manager.Options{ + DryRun: false, + Interactive: false, + Verbose: false, + } + } + + if opts.Interactive { + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + cmd.Stdin = os.Stdin + err := cmd.Run() + if err != nil { + return nil, err + } + // In interactive mode, we can't parse the output since it went to stdout + return []manager.PackageInfo{}, nil + } + out, err := cmd.Output() if err != nil { return nil, err } return ParseFindOutput(string(out), opts), nil }manager/yum/utils.go (1)
61-61:⚠️ Potential issueFix undefined variable causing compilation error.
The variable
pmis undefined in this function, which will cause a compilation error. This should use the string literal "yum" like in the other parsing functions.- PackageManager: pm, + PackageManager: "yum",
🧹 Nitpick comments (2)
manager/yum/yum.go (1)
22-31: Remove or implement unused constants.Several constants are defined but appear to be unused or have empty values. Consider removing them if they're not needed, or implement proper values if they will be used in future implementations.
These constants (
ArgsDryRun,ArgsFixBroken,ArgsPurge,ArgsAutoRemove,ArgsShowProgress) are either empty or unused. If they're placeholders for future implementation, consider adding TODO comments. Otherwise, remove them to reduce code clutter.manager/yum/utils.go (1)
53-53: Consider using camelCase for variable naming consistency.The variable
name_archuses snake_case instead of Go's conventional camelCase naming. For consistency with Go conventions, consider renaming tonameArch.- name_arch := strings.Split(parts[0], ".") - if len(name_arch) != 2 { + nameArch := strings.Split(parts[0], ".") + if len(nameArch) != 2 { continue } packageInfo := manager.PackageInfo{ - Name: name_arch[0], - Arch: name_arch[1], + Name: nameArch[0], + Arch: nameArch[1],Also applies to: 93-93
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
README.md(1 hunks)cmd/syspkg/main.go(2 hunks)manager/yum/utils.go(1 hunks)manager/yum/yum.go(1 hunks)manager/yum/yum_test.go(1 hunks)osinfo/osinfo.go(1 hunks)syspkg.go(4 hunks)syspkg_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- README.md
- osinfo/osinfo.go
- cmd/syspkg/main.go
- syspkg_test.go
🧰 Additional context used
🧠 Learnings (1)
manager/yum/yum.go (1)
Learnt from: aijanai
PR: bluet/syspkg#10
File: manager/yum/yum.go:88-100
Timestamp: 2025-05-22T22:08:45.164Z
Learning: For the syspkg project, dry run mode is not needed for read-only operations like Find (search) as they don't modify the system and have no side effects.
🔇 Additional comments (2)
syspkg.go (1)
23-29: Changes look good!The yum integration follows the existing pattern for other package managers, and the lexicographical ordering for empty package manager names provides deterministic behavior.
Also applies to: 46-47, 81-81, 103-120
manager/yum/utils.go (1)
37-37: Good improvements addressing previous feedback.The implementation shows good improvements from previous reviews:
- Regex compilation moved outside the loop for better performance
- Proper bounds checking before array access to prevent index out of range panics
- Consistent error handling for malformed package entries
These changes enhance both performance and robustness.
Also applies to: 54-56, 94-96
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
manager/yum/yum_test.go (3)
1-1: Fix incorrect file comment.The comment indicates this is
yum.gobut the file is actuallyyum_test.go.-// yum/yum.go +// yum/yum_test.go
26-26: Fix typo in error message.- t.Fatal("YumPackageManager should not support list-upgadeable") + t.Fatal("YumPackageManager should not support list-upgradeable")
131-131: Fix inverted logic in package verification.The condition uses OR (
||) when it should use AND (&&) to properly verify all package attributes match.- if pack.Name == "rpm" || pack.Arch == "x86_64" || pack.Version == "4.16.1.3-34.el9.0.1" { + if pack.Name == "rpm" && pack.Arch == "x86_64" && pack.Version == "4.16.1.3-34.el9.0.1" {manager/yum/yum.go (2)
55-82: Add DryRun support to Refresh method.The Refresh method doesn't handle the DryRun option, which should be supported even for cache cleaning operations.
func (a *PackageManager) Refresh(opts *manager.Options) error { cmd := exec.Command(pm, "clean", "expire-cache") if opts == nil { opts = &manager.Options{ DryRun: false, Interactive: false, Verbose: false, } } + if opts.DryRun { + if opts.Verbose { + log.Println("Would run:", pm, "clean", "expire-cache") + } + return nil + } if opts.Interactive { cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr cmd.Stdin = os.Stdin err := cmd.Run() return err } else { out, err := cmd.Output() if err != nil { return err } if opts.Verbose { log.Println(string(out)) } return nil } }
117-119: Fix Clean method to pass options correctly.The Clean method passes
nilto Refresh instead of forwarding the provided options.func (a *PackageManager) Clean(opts *manager.Options) error { - return a.Refresh(nil) + return a.Refresh(opts) }
🧹 Nitpick comments (1)
manager/yum/yum.go (1)
22-31: Consider removing unused constants.Many of the defined constants (ArgsDryRun, ArgsFixBroken, ArgsPurge, etc.) are empty strings and not used in the implementation. Consider removing them or adding TODO comments if they're planned for future use.
const ( ArgsAssumeYes string = "-y" ArgsAssumeNo string = "--assumeno" ArgsQuiet string = "-q" - ArgsDryRun string = "" - ArgsFixBroken string = "" - ArgsPurge string = "" - ArgsAutoRemove string = "" - ArgsShowProgress string = "" + // TODO: Add support for additional yum arguments as needed )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
manager/yum/yum.go(1 hunks)manager/yum/yum_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
manager/yum/yum.go (1)
Learnt from: aijanai
PR: bluet/syspkg#10
File: manager/yum/yum.go:88-100
Timestamp: 2025-05-22T22:08:45.164Z
Learning: For the syspkg project, dry run mode is not needed for read-only operations like Find (search) as they don't modify the system and have no side effects.
🔇 Additional comments (9)
manager/yum/yum_test.go (3)
11-40: Test function logic is correct and comprehensive.The test properly verifies that all unsupported operations (Install, Delete, ListUpgradable, Upgrade, UpgradeAll, AutoRemove) return errors, which aligns with the read-only nature of this yum implementation.
97-117: Test logic and assertions are correct.The test properly validates that the first package in the parsed output has the expected name "nginx" and architecture "x86_64".
141-167: Test validation is comprehensive and correct.The test properly validates the parsing of package info output, checking name, architecture, and version fields.
manager/yum/yum.go (6)
1-17: Package documentation is comprehensive and well-structured.The documentation clearly explains the purpose, context (YUM vs DNF), and relationship to the broader syspkg library.
47-53: Unimplemented methods correctly return errors.The Install and Delete methods appropriately return "not implemented" errors, which aligns with the read-only design goals of this PR.
84-95: Find method implementation is correct for read-only operation.Based on the retrieved learnings, DryRun support is not needed for read-only operations like Find since they have no side effects. The implementation correctly executes the search and parses the output.
97-106: ListInstalled method implementation is correct for read-only operation.Similar to Find, this read-only operation doesn't require DryRun support. The implementation correctly lists installed packages and parses the output.
121-129: GetPackageInfo method implementation is correct for read-only operation.This read-only operation correctly retrieves package information without needing DryRun support, consistent with the established pattern.
131-133: AutoRemove method correctly returns not implemented error.The method appropriately indicates that this functionality is not supported in the current read-only implementation.
…entation saying so
add yum tests (commented e2e yum tests as they work only on a RHEL and left unit)
add rockylinux/9 dist id (rocky)
… during line parsing
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
manager/yum/yum_test.go (3)
1-1: Fix incorrect file comment.
26-26: Fix typo in error message.
131-138: Fix inverted logic and incorrect error message.manager/yum/utils.go (1)
53-62:⚠️ Potential issueFix variable naming convention and undefined variable.
The code has two issues:
- Variable
name_archshould use camelCase per Go conventions- Variable
pmis undefined- name_arch := strings.Split(parts[0], ".") - if len(name_arch) != 2 { + nameArch := strings.Split(parts[0], ".") + if len(nameArch) != 2 { continue } packageInfo := manager.PackageInfo{ - Name: name_arch[0], - Arch: name_arch[1], - PackageManager: pm, + Name: nameArch[0], + Arch: nameArch[1], + PackageManager: "yum", }manager/yum/yum.go (1)
117-119: Fix: Pass options parameter to Refresh method.The
Cleanmethod should pass the providedoptsparameter toRefreshinstead ofnilto ensure caller options are properly forwarded.func (a *PackageManager) Clean(opts *manager.Options) error { - return a.Refresh(nil) + return a.Refresh(opts) }
🧹 Nitpick comments (2)
syspkg_test.go (1)
56-145: Fix formatting inconsistencies throughout the file.Please apply consistent formatting:
- Add spaces after commas:
pm,err→pm, err- Add spaces around operators:
err!=nil→err != nilRun
gofmtto automatically fix these formatting issues:gofmt -w syspkg_test.gosyspkg.go (1)
105-128: Good error handling implementation with a minor suggestion.The addition of error handling and lexicographical sorting for deterministic behavior is excellent. However, when a package manager is not found, consider explicitly returning
nilinstead of the zero-valuepmfor clarity:- pm,found := s.pms[name] - if !found { - return pm, errors.New("no such package manager") - } + pm, found := s.pms[name] + if !found { + return nil, errors.New("no such package manager") + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
README.md(1 hunks)cmd/syspkg/main.go(2 hunks)interface.go(1 hunks)manager/yum/utils.go(1 hunks)manager/yum/yum.go(1 hunks)manager/yum/yum_test.go(1 hunks)osinfo/osinfo.go(1 hunks)syspkg.go(4 hunks)syspkg_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- osinfo/osinfo.go
🧰 Additional context used
🧠 Learnings (1)
manager/yum/yum.go (1)
Learnt from: aijanai
PR: bluet/syspkg#10
File: manager/yum/yum.go:88-100
Timestamp: 2025-05-22T22:08:45.164Z
Learning: For the syspkg project, dry run mode is not needed for read-only operations like Find (search) as they don't modify the system and have no side effects.
🔇 Additional comments (13)
interface.go (1)
56-56: Good API improvement!Adding error handling to
GetPackageManageraligns with Go best practices and allows callers to properly handle cases where a package manager is not found.cmd/syspkg/main.go (2)
294-332: LGTM! CLI flag visibility updates are appropriate.Making apt, yum, flatpak, and snap visible in the CLI aligns with the PR objective of exposing yum support.
304-332: Consider the visibility of other package managers.I notice that dnf, pacman, apk, and zypper remain hidden. Is this intentional based on their current support status? If they have similar levels of support as yum, should they also be made visible?
manager/yum/yum.go (10)
1-8: LGTM! Good package documentation.The package documentation clearly describes the purpose and context of the YUM package manager implementation, including historical context about YUM being superseded by DNF.
22-31: Constants defined for future extensibility.The constants are well-organized and provide placeholders for YUM-specific command arguments. While some constants like
ArgsDryRunandArgsFixBrokenare empty strings, this is acceptable as they serve as placeholders for future implementation or YUM-specific equivalents.
37-45: LGTM! Proper availability check and package manager identification.Both methods correctly implement their responsibilities:
IsAvailableproperly checks for the YUM binary usingexec.LookPath, andGetPackageManagerreturns the appropriate identifier.
47-53: Correct "not implemented" stubs for write operations.The
InstallandDeletemethods appropriately return "not implemented" errors, which aligns with the PR objectives to provide read-only YUM support.
56-82: LGTM! Proper command execution and option handling.The
Refreshmethod correctly:
- Uses proper command argument splitting (previously fixed)
- Handles nil options with sensible defaults
- Properly manages interactive vs non-interactive execution
- Includes verbose logging when requested
85-95: LGTM! Read-only operation correctly implemented.The
Findmethod is correctly implemented as a read-only operation. Per the retrieved learnings, dry run mode is not needed for read-only operations like search since they don't modify the system and have no side effects.
98-106: LGTM! Read-only operation correctly implemented.The
ListInstalledmethod is correctly implemented as a read-only operation. Consistent with the Find method, dry run support is not needed for this read-only operation.
108-116: Correct "not implemented" stubs for write operations.The upgrade-related methods appropriately return "not implemented" errors, maintaining consistency with the read-only nature of this YUM implementation.
122-129: LGTM! Read-only operation correctly implemented.The
GetPackageInfomethod is correctly implemented as a read-only operation. Following the same pattern as other read-only methods, dry run support is not needed here.
131-133: Correct "not implemented" stub for write operation.The
AutoRemovemethod appropriately returns "not implemented" error, consistent with other write operations in this read-only YUM implementation.
| pm,err:=s.GetPackageManager("apt") | ||
|
|
||
| if err != nil && pm == nil { | ||
| pm,err:=s.GetPackageManager("snap") | ||
|
|
||
| if err!=nil && pm == nil { | ||
| pm,err:=s.GetPackageManager("flatpak") | ||
|
|
||
| if err!=nil && pm == nil { | ||
| t.Fatalf("apt, snap, or flatpak package manager not found") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix variable shadowing and improve code structure.
The nested declarations of pm,err create variable shadowing which can lead to confusion. Additionally, there are formatting inconsistencies throughout.
Apply this refactoring to avoid variable shadowing and improve readability:
- pm,err:=s.GetPackageManager("apt")
-
- if err != nil && pm == nil {
- pm,err:=s.GetPackageManager("snap")
-
- if err!=nil && pm == nil {
- pm,err:=s.GetPackageManager("flatpak")
-
- if err!=nil && pm == nil {
- t.Fatalf("apt, snap, or flatpak package manager not found")
- }
- }
- }
+ pm, err := s.GetPackageManager("apt")
+ if err == nil && pm != nil {
+ // apt found
+ } else if pm, err = s.GetPackageManager("snap"); err == nil && pm != nil {
+ // snap found
+ } else if pm, err = s.GetPackageManager("flatpak"); err == nil && pm != nil {
+ // flatpak found
+ } else {
+ t.Fatalf("apt, snap, or flatpak package manager not found")
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pm,err:=s.GetPackageManager("apt") | |
| if err != nil && pm == nil { | |
| pm,err:=s.GetPackageManager("snap") | |
| if err!=nil && pm == nil { | |
| pm,err:=s.GetPackageManager("flatpak") | |
| if err!=nil && pm == nil { | |
| t.Fatalf("apt, snap, or flatpak package manager not found") | |
| } | |
| } | |
| } | |
| pm, err := s.GetPackageManager("apt") | |
| if err == nil && pm != nil { | |
| // apt found | |
| } else if pm, err = s.GetPackageManager("snap"); err == nil && pm != nil { | |
| // snap found | |
| } else if pm, err = s.GetPackageManager("flatpak"); err == nil && pm != nil { | |
| // flatpak found | |
| } else { | |
| t.Fatalf("apt, snap, or flatpak package manager not found") | |
| } |
🤖 Prompt for AI Agents
In syspkg_test.go around lines 56 to 68, the code declares new variables pm and
err inside nested if statements, shadowing the outer variables and causing
potential confusion. Remove the redeclarations by assigning to the existing
variables, and improve code readability by flattening the nested structure. This
will prevent shadowing issues and make the flow clearer.
| pm,err:=s.GetPackageManager("chocolatey") | ||
| if err != nil && pm == nil { | ||
| pm,err:=s.GetPackageManager("scoop") | ||
| if err != nil && pm == nil { | ||
| pm,err:=s.GetPackageManager("winget") | ||
| if err != nil && pm == nil { | ||
| t.Fatalf("chocolatey, scoop, or winget package manager not found") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Inconsistent indentation in Windows package manager checks.
The indentation for the Windows package manager checks is inconsistent with the rest of the file.
Fix the indentation:
- pm,err:=s.GetPackageManager("chocolatey")
- if err != nil && pm == nil {
- pm,err:=s.GetPackageManager("scoop")
- if err != nil && pm == nil {
- pm,err:=s.GetPackageManager("winget")
- if err != nil && pm == nil {
- t.Fatalf("chocolatey, scoop, or winget package manager not found")
- }
- }
- }
+ pm, err := s.GetPackageManager("chocolatey")
+ if err != nil && pm == nil {
+ pm, err = s.GetPackageManager("scoop")
+ if err != nil && pm == nil {
+ pm, err = s.GetPackageManager("winget")
+ if err != nil && pm == nil {
+ t.Fatalf("chocolatey, scoop, or winget package manager not found")
+ }
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pm,err:=s.GetPackageManager("chocolatey") | |
| if err != nil && pm == nil { | |
| pm,err:=s.GetPackageManager("scoop") | |
| if err != nil && pm == nil { | |
| pm,err:=s.GetPackageManager("winget") | |
| if err != nil && pm == nil { | |
| t.Fatalf("chocolatey, scoop, or winget package manager not found") | |
| } | |
| } | |
| } | |
| pm, err := s.GetPackageManager("chocolatey") | |
| if err != nil && pm == nil { | |
| pm, err = s.GetPackageManager("scoop") | |
| if err != nil && pm == nil { | |
| pm, err = s.GetPackageManager("winget") | |
| if err != nil && pm == nil { | |
| t.Fatalf("chocolatey, scoop, or winget package manager not found") | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In syspkg_test.go between lines 128 and 137, the indentation for the nested
package manager checks is inconsistent with the rest of the file. Correct the
indentation to ensure all nested if statements are properly aligned, making the
code more readable and maintaining style consistency.
| name_arch := strings.Split(parts[0], ".") | ||
| if len(name_arch) != 2 { | ||
| continue | ||
| } | ||
| name := name_arch[0] | ||
| arch := name_arch[1] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix variable naming convention.
Variable name_arch should use camelCase per Go conventions:
- name_arch := strings.Split(parts[0], ".")
- if len(name_arch) != 2 {
+ nameArch := strings.Split(parts[0], ".")
+ if len(nameArch) != 2 {
continue
}
- name := name_arch[0]
- arch := name_arch[1]
+ name := nameArch[0]
+ arch := nameArch[1]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| name_arch := strings.Split(parts[0], ".") | |
| if len(name_arch) != 2 { | |
| continue | |
| } | |
| name := name_arch[0] | |
| arch := name_arch[1] | |
| nameArch := strings.Split(parts[0], ".") | |
| if len(nameArch) != 2 { | |
| continue | |
| } | |
| name := nameArch[0] | |
| arch := nameArch[1] |
🤖 Prompt for AI Agents
In manager/yum/utils.go between lines 93 and 98, the variable name `name_arch`
does not follow Go naming conventions. Rename `name_arch` to `nameArch` to
adhere to camelCase style. Update all references to this variable accordingly.
Address all review comments from PR #10: - Fix parsing panics by changing bounds check from \!= 2 to < 2 - Rename snake_case variables to camelCase (name_arch → nameArch) - Move regex compilation outside loop for better performance - Add context with proper timeouts (3min for reads, 5min for clean) - Update constants to reflect YUM's actual capabilities - Implement Clean method with 'yum clean all' instead of calling Refresh - Fix grammar in comments (an Go → a Go, Centos → CentOS) - Add documentation for unused opts parameters - Fix test typo: list-upgadeable → list-upgradable 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Add multi-OS testing infrastructure and comprehensive documentation: YUM Implementation Fixes (PR #14): - Fix all parsing panics and performance issues - Add context with proper timeouts - Implement correct Clean/Refresh behavior - Address all PR #10 review comments Multi-OS Testing Infrastructure: - Docker containers for Rocky Linux, AlmaLinux, Fedora, Alpine - Environment-aware test execution with smart OS detection - Test fixture generation from real package managers - Enhanced CI/CD with multi-OS matrix testing Comprehensive Documentation: - CONTRIBUTING.md: Complete developer guide with decision trees - Clear test usage scenarios with timing expectations - Practical examples for different development workflows - Quick reference tables and golden rules Key Features: - make test: Smart OS-aware testing (30s) - make test-docker-rocky: YUM on Rocky Linux (5min) - make test-docker-all: Comprehensive validation (15min) - Automatic test skipping based on available package managers 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Follow-up to PR #10: Fix YUM package manager review issues
2 participants
add read only yum support (yum search, yum info). All the other methods return an error with "not implemented".
add yum tests
Important
Add read-only YUM support for search, list installed, and get package info, with CLI exposure and tests.
search,list installed, andget package infoinyum.go.main.go.yum_test.go.README.mdto reflect YUM support for search, list installed, and get package info.This description was created by
for 56199ea. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
yumpackage implementing the package manager interface, with methods for package operations and output parsing.