-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
extract lb configuration steps into method #1841
Conversation
dee35ba
to
0a77b88
Compare
server/server.go
Outdated
log.Errorf("Error parsing server URL %s: %v", server.URL, err) | ||
return err | ||
} | ||
log.Debugf("Creating server %s at %s with weight %d", serverName, url.String(), server.Weight) |
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.
I know this is not your code but could you:
- rename
url
toURL
- remove the unnecessary
.String()
call.
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.
Sure, 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.
rename url to URL
This may give readers the false impression that URL
is an exported identifier. Now you basically have to track down the scope of the variable and realize that it's defined on the stack. 🤔
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.
I think Timo has a good point here. Additionally I did not find any written conventions in the go documentation or in the formatting tools. Therefore I would also vote to keep it lowercase, to not confuse readers. WDYT @ldez?
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.
I suggested that only to fix name conflict with a package.
You can rename to u
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.
I renamed it now to u
.
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.
LGTM
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.
LGTM
80a5f88
to
d67a994
Compare
d67a994
to
020149a
Compare
This is a small PR that does a little code cleanup in regards to the load balancer configuration. It extracts the initial configuration into a method and therefore removes code duplication.
One thing to note is that before the healthcheck configuration was setup multiple times for the Drr LB. This was not a problem as the result was always written into the same map key, though unnecessary.
@timoreimann PTAL.