Skip to content

Merge rules in IngressRoute definitions where possible#12

Open
hakkibagci wants to merge 3 commits intotraefik:masterfrom
hakkibagci:concise_ingress_routes
Open

Merge rules in IngressRoute definitions where possible#12
hakkibagci wants to merge 3 commits intotraefik:masterfrom
hakkibagci:concise_ingress_routes

Conversation

@hakkibagci
Copy link

instead of creating a route for each path, merge rules that target the same service where possible, which reduces the number of routes created and results a much more readable IngressRoute definition.
This is especially useful when there are multiple hosts with the same set of paths, I think which is a common scenario in real life.

@ldez
Copy link
Contributor

ldez commented Mar 19, 2020

Hello,

could you modify existing tests to keep the same behavior instead of applying the merge on it (i.e. modify the host)

@ldez
Copy link
Contributor

ldez commented Mar 19, 2020

after reflection I think there is a problem because you do not take into account the differences which are defined by the annotations.

I think it is better not to try to merge the ingress.

@hakkibagci
Copy link
Author

after reflection I think there is a problem because you do not take into account the differences which are defined by the annotations.

I think it is better not to try to merge the ingress.

Hey, thanks for the quick review. I think it takes into account the changes which are defined by annotations as well. Can you please give an example where it should fail, so that I can fix it?

@hakkibagci
Copy link
Author

Hello,

could you modify existing tests to keep the same behavior instead of applying the merge on it (i.e. modify the host)

I already modified the existing tests and also add some more examples which covers some edge cases. Can you please elaborate more on this? Thanks

@ldez
Copy link
Contributor

ldez commented Mar 19, 2020

I already modified the existing tests and also add some more examples which covers some edge cases

An example of a change:
https://github.com/containous/traefik-migration-tool/pull/12/files?file-filters%5B%5D=.go#diff-19d3a047b1d139a00dfef33903cc8737R11

instead of changing rule, you have to change the host.

@hakkibagci
Copy link
Author

I already modified the existing tests and also add some more examples which covers some edge cases

An example of a change:
https://github.com/containous/traefik-migration-tool/pull/12/files?file-filters%5B%5D=.go#diff-19d3a047b1d139a00dfef33903cc8737R11

instead of changing rule, you have to change the host.

We are talking about only the test yml files here, right? If so, I still don't get it because in case of there are multiple paths for the same host (for instance in case of ingress.yml) the result will be only 1 rule instead of 2. So the existing test files should be adjusted as well. How can I achieve this by changing the host and not the rule?

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.

3 participants