-
-
Couldn't load subscription status.
- Fork 49
feat: support newline and comma-whitespace external host lists for auto-discovery #468
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
Conversation
|
@itzg please have a look |
I already get emails for all PRs for all my repos. No need to at-mention me especially since it induces a notification. |
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.
Thanks for contributing this! Just a few things to tweak.
cmd/mc-router/main.go
Outdated
| wg, ctx := errgroup.WithContext(ctx) | ||
| wg.Go(func() error { | ||
| s.Run() | ||
| return nil | ||
| }) |
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.
This change is unrelated. Please remove it.
library.go
Outdated
|
|
||
| import ( | ||
| "strings" | ||
| ) | ||
|
|
||
| // SplitExternalHosts splits a string containing external hostnames by comma and/or newline delimiters. | ||
| // It trims whitespace around each hostname and filters out empty strings. | ||
| // Examples: | ||
| // - "host1.com,host2.com" -> ["host1.com", "host2.com"] | ||
| // - "host1.com, host2.com" -> ["host1.com", "host2.com"] | ||
| // - "host1.com\nhost2.com" -> ["host1.com", "host2.com"] | ||
| // - "host1.com,\nhost2.com" -> ["host1.com", "host2.com"] | ||
| func SplitExternalHosts(s string) []string { | ||
| // First, replace newlines with commas to normalize the delimiter | ||
| normalized := strings.ReplaceAll(s, "\n", ",") | ||
|
|
||
| // Split by comma | ||
| parts := strings.Split(normalized, ",") | ||
|
|
||
| // Trim whitespace and filter out empty strings | ||
| result := make([]string, 0, len(parts)) | ||
| for _, part := range parts { | ||
| trimmed := strings.TrimSpace(part) | ||
| if trimmed != "" { | ||
| result = append(result, trimmed) | ||
| } | ||
| } | ||
|
|
||
| return result | ||
| } |
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.
Please move this into the server package. I really should have included a comment in this file to say its only a placeholder to ensure the repo can be used as a library.
library.go
Outdated
| // First, replace newlines with commas to normalize the delimiter | ||
| normalized := strings.ReplaceAll(s, "\n", ",") | ||
|
|
||
| // Split by comma | ||
| parts := strings.Split(normalized, ",") |
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.
Please use https://pkg.go.dev/regexp@go1.25.3#Regexp.Split where the regex can then be ",|\n"
go.mod
Outdated
| go 1.24.0 | ||
|
|
||
| toolchain go1.24.9 |
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.
Please revert this
|
made the requested changes. moved the SplitExternalHosts to utils file in server. Can you please merge with the hacktober tags. Thanks |
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.
Super close now. Thanks.
server/utils.go
Outdated
| // - "host1.com,\nhost2.com" -> ["host1.com", "host2.com"] | ||
| func SplitExternalHosts(s string) []string { | ||
| // Use regexp to split on either comma or newline | ||
| re := regexp.MustCompile(",|\n") |
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.
The MustCompile made me realize this would be best declared as a const. Then it can be slightly optimized.
|
is this fine? |
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.
Code changes look great. Now I noticed the extra whitespace changes in README.
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.
Oh...GitHub was keeping this file collapsed due to the large diffs. Please revert all of the whitespace changes and leave just the lines that need modifying. You might need to temporarily disable code formatting in your editor.
|
Should be done now. Let me know if there are any other issues I can pickup and get done |
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.
Thanks for contributing this and review fixes.
#461
PR enhances the auto-discovery features for Docker, Docker Swarm, and Kubernetes by allowing external hostnames to be specified using newline delimiters and by trimming whitespace around comma delimiters.