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

allow using host/path pattern in httpso host field #1

Closed

Conversation

similark
Copy link

As I mentioned in your PR - this seems to allow setting host/path pattern in the HttpSO.Spec.Host field practically differentiating applications having same host but different path prefixes.

@t0rr3sp3dr0
Copy link
Owner

The idea to support path-based routing is not to append the path to the Host filed but to have a new field named Path that would contain that prefix and route accordingly. Your patch may work, but that's not the proper way of implementing this feature.

@t0rr3sp3dr0 t0rr3sp3dr0 force-pushed the pedrotorres/issue-605 branch 22 times, most recently from 8633df7 to 335fbcf Compare May 12, 2023 08:20
@similark
Copy link
Author

Can you think of other implementation changes that need to be done other than setting a new field called path to the Httpscaledobject CRD and then concatenating it to the host field for the lookup etc. as I do here? because if not, then renaming the field from host to hostPathPattern for clarity might still sound reasonable, right?

@t0rr3sp3dr0
Copy link
Owner

Renaming the field is a breaking change and it would need to be done on a new API Version. And if we are creating a new API Version, we should be doing things the proper way.

@t0rr3sp3dr0 t0rr3sp3dr0 force-pushed the pedrotorres/issue-605 branch 3 times, most recently from 77ea3cc to b3a1886 Compare May 16, 2023 03:48
@t0rr3sp3dr0 t0rr3sp3dr0 force-pushed the pedrotorres/issue-605 branch 12 times, most recently from 4257e5d to f9dc51c Compare May 17, 2023 10:31
@similark similark closed this May 21, 2023
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.

2 participants