Allow traefik.port to not be in the list of marathon ports #1309
Conversation
provider/marathon_test.go
Outdated
| @@ -551,7 +551,7 @@ func TestMarathonTaskFilter(t *testing.T) { | |||
| }, | |||
| }, | |||
| }, | |||
| expected: false, | |||
| expected: true, | |||
There was a problem hiding this comment.
Instead of flipping the expected flag, I think we should remove the entire test case as we have removed the restriction to specify a known port only.
There was a problem hiding this comment.
I think the specify-port-number test case (lines 520 ff.) will cover the updated functionality. Could you please update it in the following regards:
- Change the label port from 80 to 8080 (line 530).
- Remove the unneeded application ports (complete line 522).
There was a problem hiding this comment.
Makes a ton of sense i'll rebase into one commit shortly
There was a problem hiding this comment.
I made this change. I wasn't sure how these tests work but this passes now. Does it read the application section then expect the task section?
There was a problem hiding this comment.
If you look at end of the test case definitions, you can see what it does: For each test case, it directly calls the filter function passing our test fixtures (i.e., the application and task structs we set up) and expects the filtering to apply (or not apply) depending on the expected outcome.
There was a problem hiding this comment.
Sorry about that. I redid the commit. Now not removing that test case.
provider/marathon_test.go
Outdated
| @@ -519,57 +519,19 @@ func TestMarathonTaskFilter(t *testing.T) { | |||
| { | |||
| task: marathon.Task{ | |||
| AppID: "specify-port-number", | |||
| Ports: []int{80, 443}, | |||
| Ports: []int{8080}, | |||
| }, | |||
| applications: &marathon.Applications{ | |||
| Apps: []marathon.Application{ | |||
| { | |||
| ID: "specify-port-number", | |||
| Ports: []int{80, 443}, | |||
There was a problem hiding this comment.
Please remove the application ports. They are not needed for the test.
provider/marathon_test.go
Outdated
| applications: &marathon.Applications{ | ||
| Apps: []marathon.Application{ | ||
| { | ||
| ID: "specify-port-index", |
There was a problem hiding this comment.
I believe you removed a bit too much: The index functionality is still there, hence this test case should stay.
There was a problem hiding this comment.
I fixed this up. The ports are required so it doesn't get filtered from this section of that function: https://github.com/containous/traefik/blob/master/provider/marathon.go#L199
There was a problem hiding this comment.
The processPorts() function returns a non-empty list of ports if there are task ports. Since we define task ports during our test fixture, we should be able to make the test pass even without the application ports, shouldn't we?
In fact, I don't see application ports being used anywhere in the production code, so I'm suspecting they are a relict that could be removed from all tests. For now though, I'd prefer them to be removed from the tests we touch and leave a possibly wider cleanup to a separate PR.
There was a problem hiding this comment.
I also guess you meant to send your reply to my other comment. This comment is about the specify-port-index being removed. Could you still address it please?
|
|
Fixes #261
This is needed when using --host and the port cannot be provided to the marathon app.