add envoy gateway e2e#372
Conversation
Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com>
|
Skipping CI for Draft Pull Request. |
Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com>
|
/approve cc @johananl |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kkk777-7, Stevenjin8 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
c @johananl could you take alook |
Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com>
Stevenjin8
left a comment
There was a problem hiding this comment.
/lgtm
/hold
In case you want to change how crds work and to give @johananl a chance to take a look
Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com>
johananl
left a comment
There was a problem hiding this comment.
Thanks!
Other than the comment about the GWAPI CRD consts this looks great.
| func deployCRDs(ctx context.Context, l logger, client *apiextensionsclientset.Clientset, skipCleanup bool) (func(), error) { | ||
| l.Logf("Fetching manifests from %s", gatewayAPIInstallURL) | ||
| yamlData, err := fetchManifests(ctx, l) | ||
| func deployCRDs(ctx context.Context, l logger, client *apiextensionsclientset.Clientset, url string, skipCleanup bool) (func(), error) { |
There was a problem hiding this comment.
I understand this change, there is a subtlety here: Before the change, this function was meant to be a helper fro installing the GWAPI CRDs specifically whereas now it's a generic helper for installing arbitrary CRDs.
What I suggest we do is move the gatewayAPIVersion and gatewayAPIInstallURL consts to gwapi.go. This way we turn crd.go into a truly generic helper file while keeping GWAPI-specific stuff in one place.
There was a problem hiding this comment.
thanks, that makes sense! I've fixed.
| _, err = gwClient.GatewayV1().GatewayClasses().Update(ctx, gc, metav1.UpdateOptions{}) | ||
| } | ||
| if err != nil { | ||
| cleanupCRDs() |
There was a problem hiding this comment.
We're leaking the Helm chart here. This isn't a huge deal since we seem to have a general problem with this pattern (see #386) so please treat this comment as an FYI.
There was a problem hiding this comment.
thanks, good catch!
After #366 merged, I'll tackle this issue when I have time.
Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com>
|
/lgtm |
|
Merged! Thanks for the contribution @kkk777-7 🙏 |
What type of PR is this?
/kind test
What this PR does / why we need it:
Add Envoy Gateway E2E test.
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: