feat: add Gateway UDPRoute #1278
Conversation
|
@tao12345666333 Could you please help me rerun CI again? The |
|
sure |
Codecov Report
@@ Coverage Diff @@
## master #1278 +/- ##
==========================================
- Coverage 40.73% 40.47% -0.27%
==========================================
Files 77 78 +1
Lines 7030 7076 +46
==========================================
Hits 2864 2864
- Misses 3851 3897 +46
Partials 315 315
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
@stillfox-lee could you please resolve the conflicts? thanks! |
|
Some CI tasks failed, do you have time to deal with them? |
I will fix it ASAP. |
|
@tao12345666333 I think this PR is ready for review. |
test/e2e/scaffold/gateway.go
Outdated
| client, err := s.getGatewayClientset() | ||
| assert.Nil(ginkgo.GinkgoT(), err, "get GatewayClientset failed") | ||
| route, err := client.GatewayV1alpha2().UDPRoutes(s.namespace).Get(context.TODO(), name, metav1.GetOptions{}) | ||
| assert.Nil(ginkgo.GinkgoT(), err, "get UDPRoute failed") |
There was a problem hiding this comment.
Usually we don't check it, instead we check whether the corresponding resource is generated in APISIX
There was a problem hiding this comment.
I think the semantics of CreateUDPRoute is make sure create UDPRoute successfully. If we don't check the error here, then we need to return that error and change signature like:
func (s *Scaffold) CreateUDPRoute(...) (*gatewayv1alpha2.UDPRoute, error)
And then the caller still need to check the error which CreateUDPRoute returned. Every test case which create UDPRoute need to check that error. And that will make our code more boilerplate.
So I suggest we keep this semantics. What do you think?
There was a problem hiding this comment.
sorry for delay!
udpRoute := fmt.Sprintf(_udpRouteTemplate, name, backendName, backendPort)
err := s.CreateResourceFromString(udpRoute)
assert.Nil(ginkgo.GinkgoT(), err, "create UDPRoute failed")In fact we have done error checking here. So we don't need to do this repeatedly
There was a problem hiding this comment.
At the same time I don't think it is necessary to add CreateUDPRoute function,
CreateResourceFromString is enough, right?
There was a problem hiding this comment.
At the same time I don't think it is necessary to add
CreateUDPRoutefunction,CreateResourceFromStringis enough, right?
The function CreateUDPRoute is mainly intended to be able to reuse code. Otherwise, other test-cases will need to copy-paste the YAML template when they need to create a UDPRoute.
There was a problem hiding this comment.
These templates are all replaced by passing variables, like:
udpRoute := fmt.Sprintf(_udpRouteTemplate, name, backendName, backendPort)I don't see a clear benefit from it
There was a problem hiding this comment.
But it won't be a key to blocking this PR merge.
The main thing at the moment is the need to resolve conflicts so we can move forward
There was a problem hiding this comment.
I got it. We can just use udpRoute := fmt.Sprintf(_udpRouteTemplate, name, backendName, backendPort) instead of CreateUDPRoute. And error checking should be the responsibility of the creator.
|
Except for the comments above, everything else is LGTM. Then can merge this one. |
stillfox-lee
requested review from
tao12345666333 and
lingsamuel
and removed request for
tao12345666333 and
lingsamuel
|
@tao12345666333 It looks like CI needs your approval to run. |
@tao12345666333, Please approve the CI. It's weird that GitHub always identifies me as first time contributor. |
|
This is a known issue and I'm already looking for a solution |
|
@tao12345666333 Please trigger CI again... |
|
The failed test case seems not related to this PR. |
|
all passed! |
Type of change:
What this PR does / why we need it:
Just implement basic usage. Need other PR to support UDPRoute weight feature after Upstream can create nodes by weight.