Skip to content
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

Merged
merged 1 commit into from Jul 10, 2017

Conversation

mjeri
Copy link
Contributor

@mjeri mjeri commented Jul 8, 2017

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.

@mjeri mjeri force-pushed the refactor-lb-configuration branch from dee35ba to 0a77b88 Compare July 8, 2017 08:40
@ldez ldez added kind/enhancement a new or improved feature. status/2-needs-review labels Jul 8, 2017
@ldez ldez added this to the 1.4 milestone Jul 8, 2017
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)
Copy link
Member

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 to URL
  • remove the unnecessary .String() call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, done.

Copy link
Contributor

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. 🤔

Copy link
Contributor Author

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?

Copy link
Member

@ldez ldez Jul 9, 2017

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

Copy link
Contributor Author

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.

Copy link
Member

@ldez ldez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@juliens juliens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@juliens juliens merged commit 58ffea6 into traefik:master Jul 10, 2017
@mjeri mjeri deleted the refactor-lb-configuration branch July 10, 2017 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement a new or improved feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants