-
Notifications
You must be signed in to change notification settings - Fork 588
conformance: add test for HTTPRoute hostname precedence #4017
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
base: main
Are you sure you want to change the base?
Conversation
@robscott If you want to move this test to |
/retest |
That looks like how this should work, thanks @EyalPazz. I'll leave a conformance reviewer to lgtm though. /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: EyalPazz, youngnick The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/assign |
|
||
var HTTPRouteHostnamePrecedence = suite.ConformanceTest{ | ||
ShortName: "HTTPRouteHostnamePrecedence", | ||
Description: "An HTTPRoute with a specified hostname, and a non-specified one", |
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.
Description needs a little more detail. What it does, and what is the expected outcome of the test? Look at an example here:
Description: "A BackendTLSPolicy in the gateway-conformance-infra namespace should update the observedGeneration in all of it's Status.Conditions after an update to the spec", |
small comment on the test description, can be fixed on a followup Thanks! |
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.
I verified the tests using the Airlock Microgateway. However, I’m not sure whether this should be defined as a separate test case or if it would be better integrated into the HTTPRouteHostnameIntersection
test.
Yea i'm not sure too, wdyt @robscott |
New changes are detected. LGTM label has been removed. |
fixed the description @rikatz |
another pass, lgtm. Will wait for Rob to answer if this should be moved to the same test as |
Thanks for working on this @EyalPazz! While this looks like a great start, I think we want to be more thorough with the test cases here. For example, in the case where we have /cc @kl52752 |
+1 to test case suggested by Rob with same length of hostname. Also we need to compare this rule with other precedences like timestamp and verifies that this rule takes precedence over others. https://gateway-api.sigs.k8s.io/reference/spec/#httproutespec |
/kind test
/area conformance-test
Fixes #3921