-
Notifications
You must be signed in to change notification settings - Fork 15
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
Add Kuadrantctl tests #463
Conversation
18dd914
to
72c2aec
Compare
Sorry for long comment, I was lazy/unable to identify pieces of code that these bulletpoints relate to
I suspect it is due to the fact that the RLPs are identical and HTTPRoute is re-used across the tests so there might be a clash time to time. Potential solution might be to make the duration shorter (10s or even less) or maybe re-create the HTTPRoute as well or maybe do not have the RLPs identical (might be enough to have some random string as part of the name or smt like that)
|
This is plausible because tests are running in parallel
That is out of my control, I apply exactly what I get from the kuadrantctl and as far as I know there is not option to specify name
I will investigate that one, it could be because that route
Will try it out, so far I only tested released version |
|
I do not touch that file at all, not sure if the change needs to be done here |
6590439
to
778c3a2
Compare
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.
This time all the tests passed using kuadrantctl built from current main
branch but all the tests failed using kuadrantctl v0.2.3 (current GA). Exactly the opposite as it was before the force push (same error resource name may not be empty
). I would hesitate merging this if not working on both (once 0.2.4 is GA'd we can merge imo, not sure about the timeline though).
I tried several times, I did not encounter the RLP issue I mentioned (unexpected 429s). It seems that re-creating the HTTPRoute indeed fixed the issue.
Sadly, that is not easily achievable. There is a breaking change (Kuadrant/kuadrantctl#78) that changes a lot (where exactly the extension is specified) and I do not think it warrants the time investment and complexity needed to make it interchangable. However, we now have https://github.com/Kuadrant/kuadrantctl/releases/tag/v0.3.0, which is now GA |
Cool, I am happy to approve once you address the two comments then! |
@azgabur please review. I want you to take a look at this PR. |
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 am far from being and expert but this is good to go afaict. It works with kuadrantctl v0.3.0 (current GA).
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.
LGTM, in this state it can be merged.
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 have no idea what you force pushed, but LGTM still, I tried again and it worked (all tests passed).
Removed committed |
tests/kuadrantctl
and targetmake kuadrantctl
Customization done in two partsbase_oas
andoas_kuadrant
fixturesbase_oas
is for editing pure OAS specs, i.e adding security constrainsoas_kuadrant
is for adding x-kuadrant fieldsReviewable but not mergable due to a few outstanding bugs and enhancements:Gonna be merged even with temporary fixes due to outstandiong issues with
kuadrantctl
itselfKuadrant/kuadrantctl#94, Kuadrant/kuadrantctl#91, Kuadrant/kuadrantctl@d198cc8
Fixes #434 #365