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

Check for explicitly defined Marathon port first. #1474

Merged
merged 2 commits into from
Apr 26, 2017

Conversation

timoreimann
Copy link
Contributor

@timoreimann timoreimann commented Apr 20, 2017

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.

@timoreimann
Copy link
Contributor Author

The initial push contains an updated test only to demonstrate the problem. Once CI goes legitimately red, the fix will be pushed.

@timoreimann timoreimann force-pushed the marathon-check-port-label-overwrite-earlier branch 2 times, most recently from a9b6d4f to a537647 Compare April 20, 2017 23:32
@timoreimann timoreimann added the priority/P1 need to be fixed in next release label Apr 21, 2017
@timoreimann
Copy link
Contributor Author

@containous/traefik we should get this into 1.3 as the traefik.port label override is currently broken with Marathon.

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.

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 👼

  1. We really need to enhance these tests (mainly removing noise just like we did on docker provider)
  2. @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 🤔 )

@timoreimann
Copy link
Contributor Author

@vdemeester

  1. I plan to follow your good example and do some cleanup on Marathon. I need to add support for readiness checks in the foreseeable future, which will require doing requests against the Marathon API slightly different (it'll be one combined request per event update instead of two (apps + tasks)). This will require overhauling the tests anyway, so I'm going to take that opportunity.
  2. You're definitely right, thanks for the reminder. Will do so right away.

@ttr: Sorry for the noise (again) -- I should sync my Slack and Github handles in the future. :-)

@timoreimann timoreimann force-pushed the marathon-check-port-label-overwrite-earlier branch 2 times, most recently from 992fabb to cb7f4bd Compare April 22, 2017 21:02
@ldez ldez added kind/bug/fix a bug fix and removed bug labels Apr 25, 2017
@ldez ldez removed the priority/P1 need to be fixed in next release label Apr 25, 2017
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.
@timoreimann timoreimann force-pushed the marathon-check-port-label-overwrite-earlier branch from cb7f4bd to 099d605 Compare April 25, 2017 21:18
@timoreimann timoreimann merged commit 750fa22 into master Apr 26, 2017
@timoreimann timoreimann removed the request for review from trecloux April 26, 2017 01:26
@ldez ldez deleted the marathon-check-port-label-overwrite-earlier branch April 28, 2017 20:39
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.

3 participants