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

Implement Kubernetes Selectors, minor kube endpoint fix #516

Merged
merged 2 commits into from
Jul 13, 2016

Conversation

pnegahdar
Copy link
Contributor

No description provided.

@pnegahdar pnegahdar changed the title Selector Implement Kubernetes Selectors, minor kube endpoint fix Jul 12, 2016
@errm
Copy link
Contributor

errm commented Jul 12, 2016

Wow awesome thanks..

Might we be able to validate the label selector?

Oh and the build is failing because you need to run gofmt...

@pnegahdar
Copy link
Contributor Author

Yeah as I mentioned in the issue the selectors have a special syntax that would be difficult to parse without importing the kube code.

http://kubernetes.io/docs/user-guide/labels/

@emilevauge
Copy link
Member

emilevauge commented Jul 12, 2016

I tend to agree with @pnegahdar to NOT import k8s code 😄
Great PR btw!
Tests are still failing because of go lint: it would be better to test locally before using make binary test-integration.

if err != nil {
return watchCh, errCh, fmt.Errorf("Unable to construct query args")
}
request := c.request(url, string(queryData))
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks very similar to L57-65 could we extract a function to dry it up a little.

@vdemeester
Copy link
Contributor

@pnegahdar could you squash your commit too 👼

@pnegahdar
Copy link
Contributor Author

Sure. 2 (one per issue) or just 1?

@emilevauge
Copy link
Member

One per issue is perfect :)

@errm
Copy link
Contributor

errm commented Jul 13, 2016

LGTM

@emilevauge emilevauge modified the milestone: 1.1 Jul 13, 2016
@@ -272,7 +272,8 @@ func NewTraefikDefaultPointersConfiguration() *TraefikConfiguration {
//default Kubernetes
var defaultKubernetes provider.Kubernetes
defaultKubernetes.Watch = true
defaultKubernetes.Endpoint = "http://127.0.0.1:8080"
defaultKubernetes.Endpoint = ""
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove this default value ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To know if it was explicitly provided to decide whether to use the
kubernetes provide environment variables. It falls back to the 127 default
otherwise

On Wednesday, July 13, 2016, Emile Vauge [email protected] wrote:

In configuration.go
#516 (comment):

@@ -272,7 +272,8 @@ func NewTraefikDefaultPointersConfiguration() *TraefikConfiguration {
//default Kubernetes
var defaultKubernetes provider.Kubernetes
defaultKubernetes.Watch = true

Why did you remove this default value ?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/containous/traefik/pull/516/files/9f6484a328127189009d83dd2758115ccc4529c1#r70667451,
or mute the thread
https://github.com/notifications/unsubscribe/AAy2K8s6E9x8sQMKsWvvaLn_IL666OMEks5qVRyKgaJpZM4JKDPu
.

Copy link
Member

Choose a reason for hiding this comment

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

OK thanks

@emilevauge
Copy link
Member

LGTM

@emilevauge emilevauge merged commit 94fa95d into traefik:master Jul 13, 2016
@ldez ldez added the kind/enhancement a new or improved feature. label Jun 11, 2017
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