Add APT purge support via CustomCommandArgs #36
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughDelete now builds apt remove arguments incrementally: start with base remove args, always include fix-broken and autoremove, conditionally include Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant PM as PackageManager.Delete
participant Runner as CommandRunner
Caller->>PM: Delete(packages, opts)
PM->>PM: Initialize base args ["apt","remove"]
PM->>PM: Append fixed flags (ArgsFixBroken, ArgsAutoRemove)
PM->>PM: Merge CustomCommandArgs (scan for "--purge")
alt CustomCommandArgs contains "--purge"
PM->>PM: Append "--purge"
end
PM->>PM: Append package names
PM->>PM: Create 10-minute timeout context
PM->>Runner: Run(command args, ctx)
Runner-->>PM: stdout/stderr, exit code
PM-->>Caller: Return package list or error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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. Comment |
Summary of ChangesHello @brunodam, 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! This pull request resolves an issue in the Go library's APT backend where specifying Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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. Footnotes
|
![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 1faebef in 44 seconds. Click for details.
- Reviewed
126lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2draft 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. manager/apt/apt.go:216
- Draft comment:
Good use of slices.Contains to conditionally append the purge flag based on CustomCommandArgs. Consider adding a safeguard to prevent appending duplicate '--purge' flags if the flag might already be present in the args. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. manager/apt/apt_commandrunner_test.go:288
- Draft comment:
The new tests for Delete with and without the purge flag thoroughly verify the expected command arguments. This ensures that the flag is only appended when requested. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_xJztfvbzGGc0S7KM
You can customize 
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly implements support for the --purge flag in the Delete function for the APT package manager by checking CustomCommandArgs. The logic is sound and directly addresses the issue described. The addition of tests for both cases (with and without the purge flag) is great for ensuring correctness. I have one suggestion to refactor the new tests to improve maintainability by using a table-driven approach, which will reduce code duplication.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
manager/apt/apt.go (1)
17-24: Remove standard-libraryslicesdependency (Go <1.21 build break).The repository targets Go 1.20 (
go.mod), so importing the stdlibslicespackage (introduced in Go 1.21) breaks compilation. Please inline the check instead of pulling inslices.-import ( - "context" - "log" - "os/exec" - "slices" - "strings" +import ( + "context" + "log" + "os/exec" + "strings"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
manager/apt/apt.go(2 hunks)manager/apt/apt_commandrunner_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
manager/apt/apt_commandrunner_test.go (3)
manager/command_runner.go (1)
NewMockCommandRunner(95-102)manager/apt/apt.go (2)
NewPackageManagerWithCustomRunner(79-83)ArgsPurge(41-41)manager/options.go (1)
Options(5-23)
manager/apt/apt.go (3)
manager/yum/yum.go (1)
ArgsPurge(51-51)manager/flatpak/flatpak.go (1)
ArgsPurge(36-36)manager/snap/snap.go (1)
ArgsPurge(35-35)
🔇 Additional comments (1)
manager/apt/apt_commandrunner_test.go (1)
288-372: Tests look solid.Great coverage of both purge/no-purge paths and clear expectations on the mocked command arguments.
brunodam
force-pushed
the
00001-apt-purge-support
branch
2 times, most recently
from
221a1a7 to
86de072
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
manager/apt/apt.go (1)
215-221: Optional: consider passing through additional CustomCommandArgs (whitelist).If future needs arise (e.g., other safe apt flags), consider appending a curated subset of opts.CustomCommandArgs (e.g., [--allow-change-held-packages], [--reinstall]) rather than only --purge.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
manager/apt/apt.go(1 hunks)manager/apt/apt_commandrunner_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- manager/apt/apt_commandrunner_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
manager/apt/apt.go (3)
manager/yum/yum.go (1)
ArgsPurge(51-51)manager/snap/snap.go (1)
ArgsPurge(35-35)manager/flatpak/flatpak.go (1)
ArgsPurge(36-36)
🔇 Additional comments (1)
manager/apt/apt.go (1)
215-221: LGTM:--purgeis appended only when requested and both purge/no-purge cases are covered by tests.
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for APT package purging via the CustomCommandArgs option in the Delete() function. The implementation checks for the ArgsPurge flag in custom command arguments and appends --purge to the APT command when explicitly requested, while maintaining default behavior when not specified.
- Added purge flag detection logic in the APT package manager's
Delete()method - Added comprehensive tests to verify both purge and non-purge deletion behaviors
- Preserved backward compatibility by keeping default removal behavior unchanged
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| manager/apt/apt.go | Added logic to detect and apply ArgsPurge flag from CustomCommandArgs |
| manager/apt/apt_commandrunner_test.go | Added test cases for delete operations with and without purge flag |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
brunodam
force-pushed
the
00001-apt-purge-support
branch
from
86de072 to
86cf5df
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
manager/apt/apt.go (1)
359-361: Fix printf verb for args loggingCurrent formatting prints %!s(MISSING) for []string. Use %v or join for readability.
Apply this diff:
- log.Printf("Running command: %s %s", pm, args) + log.Printf("Running command: %s %s", pm, strings.Join(args, " "))
🧹 Nitpick comments (1)
manager/apt/apt.go (1)
213-221: Comment is misleading; code only adds assume-yesThe code adds -y when non-interactive; it does not handle assume-no. Make the comment precise.
Apply this diff:
- // Add dry-run and assume-yes/no options based on opts + // Add dry-run, and assume-yes when non-interactive to avoid hanging
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
manager/apt/apt.go(1 hunks)manager/apt/apt_commandrunner_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- manager/apt/apt_commandrunner_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
manager/apt/apt.go (3)
manager/yum/yum.go (5)
ArgsFixBroken(50-50)ArgsAutoRemove(52-52)ArgsDryRun(49-49)ArgsAssumeYes(46-46)ArgsPurge(51-51)manager/snap/snap.go (5)
ArgsFixBroken(33-33)ArgsAutoRemove(36-36)ArgsDryRun(32-32)ArgsAssumeYes(30-30)ArgsPurge(35-35)manager/flatpak/flatpak.go (5)
ArgsFixBroken(34-34)ArgsAutoRemove(38-38)ArgsDryRun(33-33)ArgsAssumeYes(31-31)ArgsPurge(36-36)
🔇 Additional comments (3)
manager/apt/apt.go (3)
207-212: Correct base args and early flag assembly — LGTMStarting with "remove" and appending -f and --autoremove up-front is correct and ensures flags precede package names.
223-229: Manual scan for purge — good fix and Go 1.20 compatibleThe loop avoids slices.Contains and keeps --purge before pkgs. Matches prior review direction.
231-233: Flag ordering now correct — LGTMAppending pkgs after flags ensures apt sees --purge before package names as expected.
- Add isPurgeRequested function to check for --purge flag in CustomCommandArgs - Modify Delete method to conditionally add --purge flag when requested - Add comprehensive tests for both with and without purge scenarios - Maintain backward compatibility by keeping --purge optional Signed-off-by: Bruno De Assis Marques <bruno.marques@swirldslabs.com>
brunodam
force-pushed
the
00001-apt-purge-support
branch
from
86cf5df to
ab6e690
Compare
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 (2)
manager/apt/apt.go(1 hunks)manager/apt/apt_commandrunner_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- manager/apt/apt_commandrunner_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
|
👋 Hey @bluet, just checking in to see if you’ve had a chance to take a look at this PR. |
|
@brunodam thanks for the PR! great job 👍 |
2 participants
Issue Description
When using the Go library and calling the Delete() function in the apt backend with ArgsPurge set in CustomCommandArgs, the purge behavior is not applied. The package is removed, but config files remain (dpkg shows state rc).
This pull request updates the APT package manager logic to support an optional purge flag during package deletion, and adds comprehensive tests to verify the correct behavior when the purge flag is used or omitted. The changes improve flexibility and reliability when removing packages.
Example (Go snippet):
###Expected
The command executed should be equivalent to:
so that configuration files are also removed.
Actual
The package is removed, but config files remain:
Notes
Important
Add support for APT package purge via
CustomCommandArgsinDelete()and include tests for purge behavior.Delete()inapt.gonow supports--purgeflag viaCustomCommandArgs.ArgsPurgeis inCustomCommandArgs,--purgeis appended to APT command.ArgsPurgeis not specified.TestDelete_WithPurgeFlaginapt_commandrunner_test.goverifies--purgeflag inclusion.TestDelete_WithoutPurgeFlaginapt_commandrunner_test.goensures--purgeis not included when not specified.This description was created by
for 1faebef. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Tests