fix: do not error out if ssh config is empty #1632
Conversation
As connections can now be stored in the internal connection, a missing or invalid ssh config should not cause the entire list of connections to be empty. This change ignores the error from the ssh config and proceeds to construct the list without it.
WalkthroughThe pull request modifies the 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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 (
|
![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 comments (1)
pkg/remote/conncontroller/conncontroller.go (1)
Line range hint
779-797: Improve error handling for empty SSH configsThe function returns an error when no compatible hostnames are found, but an empty SSH config is a valid state. Consider returning an empty slice without an error in this case.
Consider this modification:
if len(errs) == len(configFiles) { errs = append([]error{fmt.Errorf("no ssh config files could be opened: ")}, errs...) return nil, errors.Join(errs...) } - if len(discoveredPatterns) == 0 { - return nil, fmt.Errorf("no compatible hostnames found in ssh config files") - } return discoveredPatterns, nil
🧹 Nitpick comments (1)
pkg/remote/conncontroller/conncontroller.go (1)
713-714: Consider improving error handling and loggingWhile ignoring the error allows the function to continue with an empty slice, which aligns with the PR objective, we should consider logging the error for debugging purposes.
Consider adding error logging:
- fromConfig, _ := GetConnectionsFromConfig() - // ignore the error and continue with an empty slice + fromConfig, err := GetConnectionsFromConfig() + if err != nil { + log.Printf("warning: error reading SSH config: %v", err) + } + // continue with empty slice if error occurred
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/remote/conncontroller/conncontroller.go(1 hunks)
🔇 Additional comments (2)
pkg/remote/conncontroller/conncontroller.go (2)
Line range hint 711-734: LGTM! Robust connection list aggregation
The function effectively aggregates connections from multiple sources while:
- Prioritizing active connections in the list order
- Maintaining connection history
- Properly deduplicating entries
- Now gracefully handling SSH config errors
Line range hint 711-734: Verify the impact of ignoring SSH config errors
Let's verify that ignoring SSH config errors doesn't affect error handling expectations in the calling code.
✅ Verification successful
Let me gather more information about the error handling in the calling code.
Let me check one more aspect about the error handling in the GetConnectionsFromConfig function to understand what errors we're ignoring.
Ignoring SSH config errors is safe in this context
The verification shows that:
GetConnectionsList()is only used inWshServer.ConnListCommand()which properly propagates errors to the caller- The ignored error from
GetConnectionsFromConfig()is handled gracefully by continuing with an empty slice, which is a reasonable fallback when SSH config files are inaccessible - The function combines multiple sources of connection information (running, connected, internal config, SSH config) where SSH config is just one optional source
- The error handling approach aligns with the function's purpose of gathering available connections while being resilient to partial configuration issues
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for calls to GetConnectionsList to verify error handling
rg -A 2 "GetConnectionsList\(\)"
# Search for any tests that might need updating
fd -e test.go -x rg -l "GetConnectionsList"
Length of output: 471
Script:
#!/bin/bash
# Check how the result is used in wshserver.go
rg -B 5 -A 10 "GetConnectionsList\(\)" pkg/wshrpc/wshserver/wshserver.go
# Check if there are any error checks in the calling code
ast-grep --pattern 'if $_ = conncontroller.GetConnectionsList(); $_ != nil { $$$ }'
# Also check for direct error handling
ast-grep --pattern '$_ := conncontroller.GetConnectionsList()
if $_ != nil {
$$$
}'
Length of output: 740
Script:
#!/bin/bash
# Check the implementation of GetConnectionsFromConfig
ast-grep --pattern 'func GetConnectionsFromConfig() ([]string, error) {
$$$
}'
# Also check if there are any tests for this function
rg -l "GetConnectionsFromConfig" pkg/remote/conncontroller/
Length of output: 970
This modifies the last fix with a warning message for debugging purposes
As connections can now be stored in the internal connection, a missing or invalid ssh config should not cause the entire list of connections to be empty. This change ignores the error from the ssh config and proceeds to construct the list without it.