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 first port by default #782

Merged
merged 3 commits into from
Nov 2, 2016
Merged

Use first port by default #782

merged 3 commits into from
Nov 2, 2016

Conversation

guilhem
Copy link
Contributor

@guilhem guilhem commented Oct 27, 2016

This feature use first port (PORT0) by default when there is multiple of ports

If no information is given, use first index of ports
@emilevauge
Copy link
Member

@guilhem I'm not sure I understand your code ^^
Where to you choose the first port by default ?

@emilevauge emilevauge modified the milestone: 1.1 Oct 28, 2016
@guilhem
Copy link
Contributor Author

guilhem commented Oct 28, 2016

@emilevauge indeed, getPort was already doing it (badly) here: https://github.com/guilhem/traefik/blob/5641af437e9242dce64e121eeae10e5869fd4d52/provider/marathon.go#L325
I didn't change behavior to not introduce other bug.

I just change filter who was removing this "supported" case.

@emilevauge
Copy link
Member

Oh indeed ;)

Copy link
Member

@emilevauge emilevauge left a comment

Choose a reason for hiding this comment

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

Thanks @guilhem
LGTM!
/cc @containous/traefik

@vdemeester
Copy link
Contributor

@guilhem @emilevauge question though, should we do that for other providers ? (like the docker one 👼)

@guilhem
Copy link
Contributor Author

guilhem commented Oct 29, 2016

anything is ok for me. :)

@emilevauge
Copy link
Member

@vdemeester

@guilhem @emilevauge question though, should we do that for other providers ? (like the docker one 👼)

Maybe yes, we should stay consistent.

@vdemeester
Copy link
Contributor

@emilevauge hum, should we do that for docker and other providers for 1.1 then ? 👼

@guilhem
Copy link
Contributor Author

guilhem commented Nov 2, 2016

@emilevauge @vdemeester done in latest commit. to be used or not

@guilhem guilhem changed the title Marathon: Use first port by default Use first port by default Nov 2, 2016
@emilevauge
Copy link
Member

@guilhem thanks for those additions :)
/cc @containous/traefik

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 🐸

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

Successfully merging this pull request may close these issues.

4 participants