fix resolver parsing #2351
fix resolver parsing #2351
Conversation
WalkthroughThe change improves resolver file parsing in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
Comment |
![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
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
runner/options.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). (8)
- GitHub Check: Test Builds (macOS-latest)
- GitHub Check: Test Builds (windows-latest)
- GitHub Check: Test Builds (ubuntu-latest)
- GitHub Check: Functional Test (ubuntu-latest)
- GitHub Check: Functional Test (windows-latest)
- GitHub Check: Functional Test (macOS-latest)
- GitHub Check: Analyze (go)
- GitHub Check: release-test
🔇 Additional comments (2)
runner/options.go (2)
737-748: Good enhancement to resolver file parsing flexibility.The addition of whitespace trimming and comma-separated value splitting makes the resolver file parser more robust and user-friendly. It now handles multiple resolver formats gracefully.
740-740: No action required. The project'sgo.modspecifies Go 1.24.1, which fully supportsstrings.SplitSeq(introduced in Go 1.23). The code is compatible with the project's minimum Go version.
| for line := range chFile { | ||
| resolvers = append(resolvers, line) | ||
| line = strings.TrimSpace(line) | ||
| if line != "" && strings.Contains(line, ",") { | ||
| for item := range strings.SplitSeq(line, ",") { | ||
| item = strings.TrimSpace(item) | ||
| if item != "" { | ||
| resolvers = append(resolvers, item) | ||
| } | ||
| } | ||
| } else { | ||
| resolvers = append(resolvers, line) | ||
| } |
There was a problem hiding this comment.
Fix empty line handling in resolver parsing.
The else branch (line 747) appends the trimmed line regardless of whether it's empty. This occurs because the condition on line 739 only checks line != "" for the comma-splitting case. Empty lines or whitespace-only lines will fall through to the else branch and be added as empty strings to the resolvers list.
🔎 Proposed fix
for line := range chFile {
line = strings.TrimSpace(line)
if line != "" && strings.Contains(line, ",") {
for item := range strings.SplitSeq(line, ",") {
item = strings.TrimSpace(item)
if item != "" {
resolvers = append(resolvers, item)
}
}
- } else {
+ } else if line != "" {
resolvers = append(resolvers, line)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for line := range chFile { | |
| resolvers = append(resolvers, line) | |
| line = strings.TrimSpace(line) | |
| if line != "" && strings.Contains(line, ",") { | |
| for item := range strings.SplitSeq(line, ",") { | |
| item = strings.TrimSpace(item) | |
| if item != "" { | |
| resolvers = append(resolvers, item) | |
| } | |
| } | |
| } else { | |
| resolvers = append(resolvers, line) | |
| } | |
| for line := range chFile { | |
| line = strings.TrimSpace(line) | |
| if line != "" && strings.Contains(line, ",") { | |
| for item := range strings.SplitSeq(line, ",") { | |
| item = strings.TrimSpace(item) | |
| if item != "" { | |
| resolvers = append(resolvers, item) | |
| } | |
| } | |
| } else if line != "" { | |
| resolvers = append(resolvers, line) | |
| } | |
| } |
🤖 Prompt for AI Agents
In runner/options.go around lines 737 to 748, the else branch will append empty
or whitespace-only lines to resolvers because only the comma-splitting branch
checks line != ""; update the logic to skip empty lines before appending by
ensuring you only append when line != "" (e.g., continue if line == ""), and
keep the existing comma-splitting behavior so that trimmed, non-empty split
items are still appended.
| } | ||
| for line := range chFile { | ||
| resolvers = append(resolvers, line) | ||
| line = strings.TrimSpace(line) |
There was a problem hiding this comment.
The issue seems unrelated to the file parsing, but rather to the file not existing on the file system (I guess maybe a typo from the user), as in fact it gets appended to the slice directly (previous line 741). The resolver file format (one resolver per line) is common to all tools, if we want to support comma separated resolvers on the same line within file, maybe we should move this to utils. What do you think?
There was a problem hiding this comment.
It makes sense. I'll open an issue in utils to track this.
There was a problem hiding this comment.
Also, just asked user to check the path and it seem this isn't a path issue #2350 (comment)
There was a problem hiding this comment.
looks like the issue persists even with this change. For some reason the path lead to a non-existing file check. Maybe we can try to enforce additional checks on the resolver arguments (ex. presence of \ or /) which are prohibited as qualified domain name, and warning out the user and using default resolvers in case or erroring out since a resolver file was requested (ex. curl has hard fail when -dns-server is used and fails, without automatic fallbacks, but maybe we should be more fault tolerant and ease automation). What do you think?
closes #2350
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.