feat(checker): check dmsetup package #141
Conversation
longhorn/longhorn-8217 Signed-off-by: Raphanus Lo <[email protected]>
WalkthroughThe changes in this pull request involve updates to the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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: 0
🧹 Outside diff range and nitpick comments (1)
pkg/local/preflight/checker.go (1)
Line range hint
102-156: Consider adding tests for the new package checksThe implementation would benefit from tests to verify that the package checks work correctly across different package managers.
Would you like me to help create test cases for the package checks?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
pkg/local/preflight/checker.go(4 hunks)
🔇 Additional comments (5)
pkg/local/preflight/checker.go (5)
102-102: LGTM: Correct package name for Apt-based systems
The addition of dmsetup package is correct for Debian/Ubuntu-based systems and aligns with the PR objectives.
138-138: LGTM: Correct package name for SUSE-based systems
The addition of device-mapper package is correct for SUSE/openSUSE systems and maintains consistency with other RPM-based distributions.
156-156: LGTM: Correct package name for Arch-based systems
The addition of device-mapper package is correct for Arch Linux systems and maintains consistency with other distributions.
Line range hint 102-156: Note: dmcrypt implementation pending
The linked issue #8217 mentions checking both dmsetup and dmcrypt utilities, but only dmsetup is implemented in this PR. Is dmcrypt planned for a separate PR?
120-120: LGTM: Correct package name for RHEL-based systems
The addition of device-mapper package is correct for RHEL/CentOS-based systems, as it provides the dmsetup utility.
✅ Verification successful
Let me try another verification approach to check the package dependencies and naming patterns in the codebase.
Based on the search results from the codebase, I can now verify the package naming. The ripgrep results show that device-mapper is consistently used across different package managers in the codebase:
- For Yum (RHEL/CentOS):
"nfs-utils", "iscsi-initiator-utils", "cryptsetup", "device-mapper" - For Zypper/Transactional Update:
"nfs-client", "open-iscsi", "cryptsetup", "device-mapper" - For Pacman:
"nfs-utils", "open-iscsi", "cryptsetup", "device-mapper"
While for Apt (Debian/Ubuntu), it uses dmsetup:
"nfs-common", "open-iscsi", "cryptsetup", "dmsetup"
This is consistent with the package naming conventions in these distributions - RHEL/CentOS and other RPM-based systems provide the dmsetup utility through the device-mapper package, while Debian/Ubuntu provide it through the dmsetup package.
LGTM: Package name is correct for RHEL-based systems
The use of device-mapper package is correct for RHEL/CentOS systems, as it provides the required dmsetup utility. The package naming is also consistent with other package managers in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that device-mapper package provides dmsetup in RHEL/CentOS
rg -A 5 "provides.*dmsetup" || echo "No direct package info found"
# Alternative check for package naming consistency across distributions
fd -t f "device-mapper|dmsetup" || echo "No package files found"
Length of output: 130
Script:
#!/bin/bash
# Search for package-related configurations and dependencies
rg -i "device-mapper|dmsetup" -A 3 -B 3
# Look for package installation or dependency patterns
ast-grep --pattern 'packages = []string{$$$}'
# Check for any documentation or comments about package requirements
fd README -t f -X cat
Length of output: 68539
|
@mergify backport v1.7.x |
✅ Backports have been created
|
Which issue(s) this PR fixes:
Issue longhorn/longhorn#8217
What this PR does / why we need it:
Add package check for device mapper.
Port from: longhorn/longhorn#8232
Special notes for your reviewer:
Additional documentation or context