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

Homogenization of the providers (part 1) #2518

Merged
merged 11 commits into from
Dec 5, 2017

Conversation

ldez
Copy link
Contributor

@ldez ldez commented Dec 4, 2017

What does this PR do?

The first step a the homogenization of the providers.

Rewrite Labels/Annotations system (provider with template)

  • Docker: map[string]string
  • Eureka: map[string]string
  • k8s: map[string]string
  • Marathon: *map[string]string
  • Rancher: map[string]string
  • Mesos: task.Labels
  • ecs: map[string]*string

more:

  • use label package (function and constant)
  • remove dead tests
  • move the build of funcMap on a dedicate file
  • add tests on label package

No-Rewrite

  • KV: BoldDB, Consul, Etcd, ZK
  • DynamoDB (no template)

Motivation

make it easier to add new label.

More

  • Added/updated tests
  • Added/updated documentation

Mjolnir

@ldez ldez force-pushed the refactor/homogenization-providers branch from 59d7949 to 1787729 Compare December 4, 2017 19:04
tmpl := p.frontEndRuleTemplate
tmpl, err := tmpl.Parse(customFrontendRule)
if err != nil {
log.Errorf("failed to parse Consul Catalog custom frontend rule: %s", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you upper case it?

var buffer bytes.Buffer
err = tmpl.Execute(&buffer, templateObjects)
if err != nil {
log.Errorf("failed to execute Consul Catalog custom frontend rule template: %s", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you upper case it?

return func(app *marathon.Application) {
app.AddLabel(types.LabelPrefix+serviceName+"."+property, value)
app.AddLabel(label.Prefix+serviceName+"."+property, value)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should not we use fmt.Sprintf?

tmpl := p.frontEndRuleTemplate
tmpl, err := tmpl.Parse(customFrontendRule)
if err != nil {
log.Errorf("failed to parse Consul Catalog custom frontend rule: %s", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you upper case it?

var buffer bytes.Buffer
err = tmpl.Execute(&buffer, templateObjects)
if err != nil {
log.Errorf("failed to execute Consul Catalog custom frontend rule template: %s", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you upper case it?

t.Fatalf("expected %#v, got %#v", c.expectedBackends, actualConfig.Backends)
}
if !reflect.DeepEqual(actualConfig.Frontends, c.expectedFrontends) {
t.Fatalf("expected %#v, got %#v", c.expectedFrontends, actualConfig.Frontends)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you upper case it?


var p = taskRecords(taskState)
if len(p) == 0 {
t.Fatal("taskRecord should return at least one task")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add the word Function at the beginning,? Thus the falal log begins with an upper case.

}

func (p *Provider) getFrontendRule(service rancherData) string {
defaultRule := "Host:" + strings.ToLower(strings.Replace(service.Name, "/", ".", -1)) + "." + p.Domain
Copy link
Contributor

Choose a reason for hiding this comment

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

Should not we use fmt.Sprintf?


data, meta, err := catalog.Services(options)
if err != nil {
log.Errorf("Failed to list services: %s", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should not we use %v instead of %s to log an err?

for key, value := range data {
nodes, _, err := catalog.Service(key, "", &api.QueryOptions{})
if err != nil {
log.Errorf("Failed to get detail of service %s: %s", key, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should not we use %v instead of %s to log an err?

Copy link
Contributor

@nmengin nmengin left a comment

Choose a reason for hiding this comment

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

LGTM 🎉 👏 🔨

@traefiker traefiker merged commit 0472d19 into traefik:master Dec 5, 2017
@ldez ldez deleted the refactor/homogenization-providers branch December 5, 2017 18:44
@emilevauge
Copy link
Member

😢 I failed to review this PR, but that's awesome job @ldez 👏 👏 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants