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

Add all available tags to Consul Catalog Backend #2646

Merged
merged 13 commits into from
Jan 4, 2018

Conversation

ldez
Copy link
Contributor

@ldez ldez commented Jan 2, 2018

What does this PR do?

  • add all available tags:
    • add passTLSCert tag.
    • add HealthCheck tags.
    • add Headers tags.
    • add error pages tags.
    • add rate limit tags.
    • add whitelistSourceRange tag.
    • add frontends redirect tags.
  • enhance template readability
  • enhance template method readability

Motivation

Homogenization of the providers [part2]: all providers must have the same options available.

More

  • Added/updated tests
  • Added/updated documentation

Additional Notes

After the review I will split consul package to consul and consulcatalog

Related to #618, #1465

Copy link
Member

@mmatur mmatur left a comment

Choose a reason for hiding this comment

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

First review, nice work @ldez 👏

desc string
name string
tags []string
defaultValue int64
Copy link
Member

Choose a reason for hiding this comment

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

defaultValue is not used in test cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

desc string
name string
tags []string
defaultValue int
Copy link
Member

Choose a reason for hiding this comment

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

defaultValue is not used in test cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

desc string
name string
tags []string
defaultValue bool
Copy link
Member

Choose a reason for hiding this comment

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

defaultValue is not used in test cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

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.

Few notes on this huge PR 👏

}
}

func (p *CatalogProvider) getHeathCheck(tags []string) *types.HealthCheck {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean getHealthCheck?

}
}

func TestCatalogProviderGetHeathCheck(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean TestCatalogProviderGetHealthCheck?

{{end}}

{{ $healthCheck := getHeathCheck $service.Attributes }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean getHealthCheck?

"getCircuitBreaker": p.getCircuitBreaker,
"getLoadBalancer": p.getLoadBalancer,
"getMaxConn": p.getMaxConn,
"getHeathCheck": p.getHeathCheck,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean getHealthCheck?

@ldez ldez force-pushed the refactor/label-consul-catalog branch from c195ea1 to 041007d Compare January 4, 2018 00:00
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 👏 👏

Copy link
Member

@mmatur mmatur 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

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

Successfully merging this pull request may close these issues.

5 participants