-
Notifications
You must be signed in to change notification settings - Fork 296
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: more robust processor checks #1656
Conversation
This ensures invalid architectures cannot have wsh installed.
WalkthroughThe pull request introduces changes across multiple files related to handling SSH command outputs and architecture detection. The primary modifications involve updating the methods A new utility function The modifications introduce a new import for the Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 🪧 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
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/util/utilfn/utilfn.go (1)
967-984
: Consider additional 32-bit architectures.
This function validates known 64-bit patterns but omits potential 32-bit (e.g. i386) or arm variants. If you intentionally restrict to x64 and arm64, this is fine. Otherwise, consider broader coverage or user feedback on failure.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/remote/connutil.go
(3 hunks)pkg/util/utilfn/utilfn.go
(1 hunks)pkg/wsl/wsl-util.go
(3 hunks)
🔇 Additional comments (10)
pkg/remote/connutil.go (5)
19-19
: New utility import looks good.
The addition of this import for utilfn
is clear and will support the new architecture filtering functionality.
149-149
: Capturing both stdout & stderr is a good improvement.
Switching to CombinedOutput
ensures better context for diagnosing errors and parsing command results.
188-190
: Architecture detection updated with error handling.
Using CombinedOutput
plus utilfn.FilterValidArch
will improve the robustness of architecture validation on Unix.
198-200
: Consistent architecture checking for Windows CMD.
Filtering the architecture string after confirming it is not a literal environment variable leads to more robust checks.
208-210
: PowerShell architecture detection is consolidated as well.
The same pattern of capturing combined output and validating ensures consistent architecture handling. Great job.
pkg/wsl/wsl-util.go (5)
20-20
: New import for architecture filtering.
Using utilfn
will streamline validation logic here.
100-100
: Use of CombinedOutput
improves OS detection logs.
This matches the updated approach: capturing stderr can help diagnose issues.
129-129
: Leveraging FilterValidArch
effectively.
This ensures your uname -m
output is normalized and validated.
133-135
: CombinedOutput
usage for Windows CMD environment variable.
Retaining the guard against literal placeholders ensures accurate architecture detection.
139-141
: PowerShell architecture handling is similarly reinforced.
Aligning this with the same robust checks is consistent and reliable.
This ensures invalid architectures cannot have wsh installed. This includes validating the output of
uname -m
and PROCESSOR_ARCHITECTURE