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

Use default frontend priority of zero. #1906

Conversation

timoreimann
Copy link
Contributor

@timoreimann timoreimann commented Jul 31, 2017

Conventionally all providers should return a priority of zero if no override is given. The server package will then compute the priority based on the specified rules (i.e., path and host components). See also here and here in the server package.

The Kubernetes provider diverges and returns the path length as default value explicitly. This change removes the custom behavior.

As a side refactoring, we change the tests to stop comparing JSON values which are prone to subtle but non-visible differences (namely, nil and empty properties are not reflected). Instead, we use assert.Equal which also does a better job at showing a readable diff.

k8s maintainers and @containous/traefik: PTAL.

@timoreimann timoreimann requested a review from a team July 31, 2017 15:44
@timoreimann timoreimann force-pushed the k8s-use-default-frontend-priority-of-zero branch from ff0b093 to f072b5f Compare July 31, 2017 15:45
@timoreimann timoreimann changed the title [kubernetes] Use default frontend priority of zero. Use default frontend priority of zero. Jul 31, 2017
@ldez ldez added this to the 1.4 milestone Jul 31, 2017
Copy link
Contributor

@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

@ldez ldez added size/S and removed size/S labels Aug 4, 2017
@ldez ldez added the size/S label Aug 16, 2017
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
Contributor

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐮

@timoreimann
Copy link
Contributor Author

timoreimann commented Aug 16, 2017

@dtomcej @errm as Kubernetes co-owners, are you ok with this PR getting merged?

@ldez
Copy link
Contributor

ldez commented Aug 18, 2017

Waiting on code owner review from containous/kubernetes.

Copy link
Contributor

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

code owner LGTM 😞

Conventionally all providers should return a priority of zero if no
override is given. The server package will then compute the priority
based on the specified rules (i.e., path and host components).

The Kubernetes provider diverges and returns the path length as
default value explicitly. This change removes the custom behavior.

As a side refactoring, we change the tests to stop comparing JSON values
which are prone to subtle but non-visible differences (namely, nil
and empty properties are not reflected). Instead, we use assert.Equal
which also does a better job at showing a readable diff.
@traefiker traefiker force-pushed the k8s-use-default-frontend-priority-of-zero branch from 9931b9c to 169343d Compare August 18, 2017 13:42
@traefiker traefiker merged commit b80ecd5 into traefik:master Aug 18, 2017
@timoreimann timoreimann deleted the k8s-use-default-frontend-priority-of-zero branch October 9, 2017 20:28
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