Add categories to GG CRDs #5006

arianaw66 · 2024-11-14T01:53:00Z

Description

Add common and enterprise categories to Gloo Gateway CRDs so that the convenient kubectl get gloo-gateway -A can be used to list all GG CRs on the cluster, and kubectl get solo-io -A can be used to list all GG enterprise CRs.

API changes

Added category/ies to CRDs

Interesting decisions

This PR follows the example of #5592, which shows that solo-kit only generates the schema section of the CRD (see #5592 (comment)). Therefore, we hardcode the new categories in the CRD yaml files.

Testing steps

I manually verified behavior by generating the helm chart and applying it, applying CRs, and running kubectl get solo-io, just as the e2e test does.

Notes for reviewers

I'd like feedback on whether the KubeGatewaySuite is the appropriate suite for the new e2e tests.

Checklist:

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works

…tegory

solo-changelog-bot · 2024-11-14T18:32:23Z

Issues linked to changelog:
https://github.com/solo-io/solo-projects/issues/6605

jenshu · 2024-11-14T19:39:41Z

install/helm/gloo/crds/gateway.gloo.solo.io_directresponses.yaml

@@ -13,6 +13,7 @@ spec:
  names:
    categories:
    - gloo-gateway


do we want to remove gloo-gateway from existing crds so we don't have 3 different possible categories?

Decided in standup to replace "k8sgateway" with "gloo-gateway". I added more context as a comment on the issue.

…t PR merged to main: https://github.com/solo-io/gloo/actions/runs/11842016564

…tegory

This reverts commit fb383e3.

…tegory

sam-heilbron

LGTM!

test/kube2e/README.md

install/helm/gloo/crds/enterprise.gloo.solo.io_v1_AuthConfig.yaml

sam-heilbron · 2024-11-15T17:37:04Z

install/helm/gloo/crds/graphql.gloo.solo.io_v1beta1_GraphQLApi.yaml

@@ -8,6 +8,9 @@ metadata:
 spec:
  group: graphql.gloo.solo.io
  names:
+    categories:
+    - solo-io


If our approach is that any enterprise CRDs will have solo-io as well, can we clarify that somewhere as an intentional decision? I can see a new CR be added, and it being challenging for a new developer to understand that expectation

Hopefully the new README helps? Other suggestions of where to surface that info?

I'm also hopeful that if the new helm test fails, the test code will help devs realize the expectation, but someone may add an enterprise CRD without the solo-io category and the test wouldn't catch that.

sam-heilbron · 2024-11-15T17:38:24Z

test/kubernetes/e2e/features/helm/types.go

+	enterpriseCRDCategory = "solo-io"
+	CommonCRDCategory     = "gloo-gateway"
+
+	enterpriseCRDs = []string{


nit: It could be useful to callout that we have other CRDs that have a mix of APIs (some open source, some enterprise) and that we don't consider those "enterprise CRDs" since they contain open source functionality

The new README hopefully clarifies a bit, but not sure where you want this called out and if that's the right place.

sam-heilbron · 2024-11-15T17:39:40Z

test/kubernetes/e2e/features/helm/types.go

@@ -18,4 +18,13 @@ var (
 			},
 		},
 	}
+
+	enterpriseCRDCategory = "solo-io"


nit: It almost feels like these are a contract of our product, and deserve to live in our codebase, and then imported into our tests. It would also address my concern about enterprise crds expecting a solo-io category as being challenging for a new dev to discover

Tbh I was a little surprised there wasn't already a list of the enterprise CRDs elsewhere in the code base, but I didn't find one. Any suggestions for where/in what format this ought to live?

there is a list in enterprise here. i don't know if it makes sense to add any more enterprise-specific code in our OSS repo (although the enterprise crds live in the oss repo so it's a little messy to begin with). maybe it makes sense to add these new enterprise-specific tests in solo-projects only?

I'm obviously biased by the additional effort of moving the tests 🙃 but I would prefer to keep the tests closer to the code changes. I'd rather see the tests fail in this repo when I add a new CRD with the wrong categories than have someone else notice a failure in solo-projects some unknown time after my gloo PR has merged.

…tegory

sam-heilbron

Great work!

test/kubernetes/e2e/features/CRD_categories/suite.go

arianaw66 · 2024-11-18T17:02:41Z

/kick

jenshu · 2024-11-18T17:50:11Z

test/kubernetes/e2e/features/helm/suite.go

@@ -81,7 +82,7 @@ func (s *testingSuite) TestApplyCRDs() {
 		if err != nil {
 			return err
 		}
-		if info.IsDir() {
+		if info.IsDir() || info.Name() == "README.md" {


ah i didn't realize the readme was added in the crds dir. i'd suggest either moving it elsewhere (if there's a good place to put it), or adding the README.md to the helmignore file so it doesn't get packaged with our helm chart

Gotcha. I was surprised codegen passed but figured it was fine if so.
I think adding it to the helmignore file is the right call, but it won't fix the test which pulls from the install/helm directory directly.

This feels like a good start, but leaves us open to future files causing the same issue. What if instead, we examine the file extension, and if it's yaml we assume it's a CRD, and if it's not we skip it?

Yeah I almost did that but I'm kinda okay with this being more strict to remind us to be thoughtful adding things to that folder? Maybe it'll prompt someone to suggest adding it to the .helmignore for instance :)

arianaw66 and others added 7 commits November 12, 2024 15:29

Test hard coding category in VS

bdcb9a0

Merge refs/heads/main into GG-CRD-category

9673bf0

add solo-io category to enterprise CRDs

b3dc93f

add to graphQL too

0b4eeba

Merge branch 'main' of https://github.com/solo-io/gloo into GG-CRD-ca…

23667ed

…tegory

add note to legacy test/kube2e README

4d97821

add e2e test

db49327

arianaw66 mentioned this pull request Nov 14, 2024

Test hard coding category in VS k8sgateway/k8sgateway#10325

Closed

4 tasks

arianaw66 added 5 commits November 14, 2024 13:04

Merge branch 'main' of https://github.com/solo-io/gloo into GG-CRD-ca…

921f2c6

…tegory

add k8sgateway category to all CRDs

3c9c800

add e2e testing

32c5936

goimports

b495fc5

changelog

db1b187

add category properly for gateway2 CRDs

4545e58

arianaw66 marked this pull request as ready for review November 14, 2024 19:13

run CRDCategories tests in CI

e6ac305

jenshu reviewed Nov 14, 2024

View reviewed changes

arianaw66 added 2 commits November 14, 2024 14:45

actually test services feature

9bf15de

update kubernetes tests timing based on most recent run of most recen…

25c06dd

…t PR merged to main: https://github.com/solo-io/gloo/actions/runs/11842016564

arianaw66 added the work in progress label Nov 14, 2024

arianaw66 added 8 commits November 14, 2024 17:32

tmp- sanity check it passes w/o rlc

fb383e3

Merge branch 'main' of https://github.com/solo-io/gloo into GG-CRD-ca…

ecc23be

…tegory

Revert "tmp- sanity check it passes w/o rlc"

f74e2fa

This reverts commit fb383e3.

refactor tests to respect OSS boundary

2f6e4fe

ugh, goimports

730a6bf

Merge branch 'main' of https://github.com/solo-io/gloo into GG-CRD-ca…

cefcd17

…tegory

change k8sgateway to gloo-gateway

72ffa84

simplify TestCommonCategory() assertion

28a4d37

sam-heilbron reviewed Nov 15, 2024

View reviewed changes

arianaw66 added 2 commits November 15, 2024 16:04

PR comments

de8fbe6

Merge branch 'main' of https://github.com/solo-io/gloo into GG-CRD-ca…

c9e1185

…tegory

sam-heilbron approved these changes Nov 15, 2024

View reviewed changes

Merge branch 'main' into GG-CRD-category

143179d

arianaw66 added keep pr updated and removed work in progress labels Nov 18, 2024

Merge branch 'main' into GG-CRD-category

726c83b

jenshu reviewed Nov 18, 2024

View reviewed changes

test/kubernetes/e2e/features/CRD_categories/suite.go Outdated Show resolved Hide resolved

jenshu approved these changes Nov 18, 2024

View reviewed changes

lowercase dir name per PR comment

a18a6d7

jenshu approved these changes Nov 18, 2024

View reviewed changes

skip unmarshalling README.md in helm test

94e21cf

jenshu reviewed Nov 18, 2024

View reviewed changes

add README.md to .helmignore

08620a6

jenshu approved these changes Nov 18, 2024

View reviewed changes

arianaw66 added 2 commits November 18, 2024 13:29

improve error and comment

f620f0f

goimports

0eee4a3

sam-heilbron approved these changes Nov 18, 2024

View reviewed changes

soloio-bulldozer bot merged commit 22e6f2c into main Nov 18, 2024
17 of 18 checks passed

soloio-bulldozer bot deleted the GG-CRD-category branch November 18, 2024 20:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add categories to GG CRDs #5006

Add categories to GG CRDs #5006

arianaw66 commented Nov 14, 2024 •

edited

Loading

solo-changelog-bot bot commented Nov 14, 2024

jenshu Nov 14, 2024

arianaw66 Nov 15, 2024

sam-heilbron left a comment

sam-heilbron Nov 15, 2024

arianaw66 Nov 15, 2024

sam-heilbron Nov 15, 2024

arianaw66 Nov 15, 2024

sam-heilbron Nov 15, 2024

arianaw66 Nov 15, 2024

jenshu Nov 15, 2024

arianaw66 Nov 18, 2024

sam-heilbron left a comment

arianaw66 commented Nov 18, 2024

jenshu Nov 18, 2024

arianaw66 Nov 18, 2024

sam-heilbron Nov 18, 2024

arianaw66 Nov 18, 2024

Add categories to GG CRDs #5006

Add categories to GG CRDs #5006

Conversation

arianaw66 commented Nov 14, 2024 • edited Loading

Description

API changes

Interesting decisions

Testing steps

Notes for reviewers

Checklist:

solo-changelog-bot bot commented Nov 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-heilbron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-heilbron left a comment

Choose a reason for hiding this comment

arianaw66 commented Nov 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arianaw66 commented Nov 14, 2024 •

edited

Loading