-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Check for explicitly defined Marathon port first. #1474
Check for explicitly defined Marathon port first. #1474
Conversation
The initial push contains an updated test only to demonstrate the problem. Once CI goes legitimately red, the fix will be pushed. |
a9b6d4f
to
a537647
Compare
@containous/traefik we should get this into 1.3 as the Sorry for the big delta in the tests, but the fact that they did not adequately address/test-cover the code is one of the reasons why the problem didn't surface earlier. Thus, I needed to shift things around. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👼
- We really need to enhance these tests (mainly removing noise just like we did on docker provider)
- @ttr you probably want to remove the first commit before we merge (otherwise it is going to break bisect if we merge it without squashing 😛 — I think 🤔 )
@ttr: Sorry for the noise (again) -- I should sync my Slack and Github handles in the future. :-) |
992fabb
to
cb7f4bd
Compare
The comma ok idiom fits better.
Previously, we did the check too late resulting in the traefik.port label not being effective. The change comes with additional refactorings in production and tests.
cb7f4bd
to
099d605
Compare
Check for explicitly defined Marathon port first.
Previously, we did the check too late resulting in the
traefik.port
label not being effective.The change comes with additional refactorings in production and tests.
Original issue at #261.
Complements PR at #1394.
Discovered along #1390.