fix(preflight): check APT package installation by status flag #132
Conversation
WalkthroughThe changes in this pull request enhance the Changes
Assessment against linked issues
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
![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
🧹 Outside diff range and nitpick comments (1)
pkg/local/preflight/packagemanager/apt.go (1)
78-81: Consider adding context to execution error.When the command execution fails, it would be helpful to wrap the error with additional context.
output, err = c.executor.Execute([]string{}, "dpkg-query", []string{"-l", name}, commontypes.ExecuteNoTimeout) if err != nil { - return + return output, fmt.Errorf("failed to query package %s: %w", name, err) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
pkg/local/preflight/packagemanager/apt.go(2 hunks)pkg/local/preflight/packagemanager/packagemanager.go(2 hunks)
🔇 Additional comments (3)
pkg/local/preflight/packagemanager/packagemanager.go (1)
4-4: LGTM! Well-structured error handling addition.
The new error variable is appropriately defined at the package level and follows Go conventions. The error message is clear and concise, which will help improve error reporting for package installation checks.
Also applies to: 23-24
pkg/local/preflight/packagemanager/apt.go (2)
70-77: LGTM! Clear and helpful documentation.
The added comment block with example output format is excellent. It clearly documents the expected format and helps understand the parsing logic.
82-92: Verify the fix addresses the nfs-common detection issue.
The implementation looks correct for detecting package installation status. Let's verify it specifically addresses the nfs-common package detection issue mentioned in the PR objectives.
COLDTURNIP
force-pushed
the
9495-dpkg_check_package_status
branch
from
5b3b5c2 to
513edfa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
pkg/local/preflight/packagemanager/apt.go (2)
11-28: LGTM: Well-documented constants improve code clarity.The new constants accurately represent dpkg package states and desired actions, making the code more maintainable.
Consider adding brief descriptions for each constant group to improve documentation:
const ( // Ref: man 1 dpkg-query + // Package desired actions dpkgDesiredActionUnknown = "u" dpkgDesiredActionInstall = "i" dpkgDesiredActionHold = "h" dpkgDesiredActionRemove = "r" dpkgDesiredActionPurge = "p" + // Package status flags dpkgStatusNotInstalled = "n" dpkgStatusConfigFiles = "c"
89-90: Document the format string interpretation.The dpkg-query format string is crucial for correct operation but lacks documentation about the expected output format.
Add a comment explaining the format string components:
// example for an installed package: // $ dpkg-query -f='${binary:Package} ${db:Status-Abbrev}' -W nfs-common // nfs-common ii +// where: +// - ${binary:Package}: package name +// - ${db:Status-Abbrev}: two-character status (e.g., 'ii' means installed)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
pkg/local/preflight/packagemanager/apt.go(2 hunks)pkg/local/preflight/packagemanager/packagemanager.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/local/preflight/packagemanager/packagemanager.go
🔇 Additional comments (3)
pkg/local/preflight/packagemanager/apt.go (3)
4-4: LGTM: Import required for string manipulation.
The addition of the strings package is necessary and properly used for parsing the dpkg-query output.
95-101:
Enhance the parsing logic robustness.
The current parsing logic could be more robust to handle various edge cases and provide better error reporting.
- fields := strings.Fields(strings.TrimSpace(output))
- if len(fields) == 2 {
- if fields[0] == name && fields[1] == dpkgDesiredActionInstall+dpkgStatusInstalled {
- return output, nil
- }
- }
- return output, packageNotInstalledError
+ fields := strings.Fields(strings.TrimSpace(output))
+ if len(fields) != 2 {
+ return output, fmt.Errorf("unexpected dpkg-query output format: %q", output)
+ }
+
+ if fields[0] != name {
+ return output, fmt.Errorf("package name mismatch: expected %q, got %q", name, fields[0])
+ }
+
+ status := fields[1]
+ if len(status) != 2 {
+ return output, fmt.Errorf("invalid status format: %q", status)
+ }
+
+ desiredAction := string(status[0])
+ pkgStatus := string(status[1])
+
+ if desiredAction == dpkgDesiredActionInstall && pkgStatus == dpkgStatusInstalled {
+ return output, nil
+ }
+
+ return output, fmt.Errorf("%w: status=%q", packageNotInstalledError, status)This enhancement:
- Provides detailed error messages for debugging
- Validates the status string format
- Separates the status components for clarity
- Includes the actual status in the error message
Likely invalid or redundant comment.
87-101: Verify package status handling aligns with PR objectives.
The PR aims to improve package status checking accuracy, but the implementation only checks for "ii" status. Consider if other status combinations should be considered as "installed".
Let's verify the package status handling in the environment check script:
Consider:
- Documenting which package statuses are considered "installed"
- Adding unit tests for different package status combinations
- Ensuring consistency with environment_check.sh behavior
✅ Verification successful
Let me gather more information about the package status handling by searching for related constants and tests.
Let me search specifically for dpkg status codes and their documentation.
Based on the search results, I can now generate the final response as we have found the complete set of dpkg status codes and their implementation.
Package status handling is correctly implemented for the use case
The current implementation checking for "ii" status (dpkgDesiredActionInstall + dpkgStatusInstalled) is the correct approach because:
- The "i" status in the first position indicates the package is desired to be installed
- The "i" status in the second position confirms the package is fully installed
- Other status combinations like "Hi" (half-installed), "Wi" (triggers awaiting), or "Fi" (half-configured) would indicate incomplete or problematic installations that shouldn't be considered as properly installed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find how environment_check.sh handles package status checks
rg -A 5 "dpkg" | grep -C 5 "nfs-common"
Length of output: 896
Script:
#!/bin/bash
# Search for dpkg status-related constants and their usage
ast-grep --pattern 'const dpkg$_'
# Search for test cases related to package status checking
rg -l "TestCheckPackageInstalled"
# Look for any documentation about package status
rg -i "package.*status" -A 3 -B 3
Length of output: 12194
Script:
#!/bin/bash
# Search for dpkg status code constants
rg -l "dpkg.*status"
# Look for any dpkg-related constants
ast-grep --pattern 'const $_ = "ii"'
# Search for any dpkg documentation or comments
rg -i "dpkg.*status.*" -A 5 -B 5
Length of output: 25232
Signed-off-by: Raphanus Lo <[email protected]>
COLDTURNIP
force-pushed
the
9495-dpkg_check_package_status
branch
from
513edfa to
d9c83f8
Compare
|
@mergify backport v1.7.x |
❌ Command disallowed due to command restrictions in the Mergify configuration.
|
|
@mergify backport v1.7.x |
✅ Backports have been created
|
Which issue(s) this PR fixes:
Issue longhorn/longhorn#9495
What this PR does / why we need it:
While checking the installation status with APT package system, we should read the actual package status instead of the exit code of
dpkg-query -lordpkg -l.Special notes for your reviewer:
Additional documentation or context