Add support for CRDs #105

Liujingfang1 · 2018-06-12T22:36:16Z

assume the crd files are in format as open API definitions
read in file content
recursively parse types and annotations and construct pathconfig
Add a unit test for parsing one CRD file and get the pathconfig
Add an integration test

mengqiy

We also should have a integration test to show the whole thing works together in a Kusomize.yaml.

mengqiy · 2018-06-14T17:01:34Z

pkg/crds/crds.go

+	return crds
+}
+
+func getCRDPathConfig(types map[string]common.OpenAPIDefinition, atype string, crd string, path []string, configs *pathConfigs) error {


line to long. break this line

mengqiy · 2018-06-14T17:03:23Z

pkg/crds/crds_test.go

+					[]transformers.PathConfig{
+						{
+							CreateIfNotPresent: false,
+							GroupVersionKind:   &schema.GroupVersionKind{Kind: "github.com/example/pkg/apis/jingfang/v1beta1.MyKind"},


schema.GroupVersionKind{Kind: "github.com/example/pkg/apis/jingfang/v1beta1.MyKind"}

This doesn't look like a correct GVK.

Changed it to MyKind

mengqiy · 2018-06-14T17:07:42Z

pkg/crds/crds.go

+			configs.AddAnnotationPathConfig(
+				transformers.PathConfig{
+					CreateIfNotPresent: false,
+					GroupVersionKind:   &schema.GroupVersionKind{Kind: crd},


IMO this doesn't construct a normal GVK.
We can build GVK from apiVersion and kind. You can use TypeMeta{}.GroupVersionKind().
apiVersion and kind info can be find in your getCRDs method.

Liujingfang1 · 2018-06-14T18:29:03Z

We also should have a integration test to show the whole thing works together in a Kusomize.yaml.

Add an integration test for

referring one core type
referring another crd type

mengqiy

Only concern is how to get the GVK for a CRD.

mengqiy · 2018-06-15T06:58:37Z

pkg/crds/crds.go

+}
+
+// getCRDs get all CRD types
+func getCRDs(types map[string]common.OpenAPIDefinition) []string {


I think the signature of this method should be
func getCRDs(types map[string]common.OpenAPIDefinition) []GroupVersionKind
As I mentioned in my earlier comment, we can use TypeMeta{APIVersion: apiVersion, Kind: kind}.GroupVersionKind() to get a GVK.
Each GVK should map to a CRD.

How about this
func getCRDs(types map[string]common.OpenAPIDefinition) map[string]GroupVersionKind
I need that the string name such as "github.com/example/pkg/apis/jingfang/v1beta1.MyKind" to find the corresponding openAPIdefinition in getCRDPathConfig. If we return a map, we have both the string name and GVK available.

mengqiy · 2018-06-15T07:00:13Z

pkg/crds/crds.go

+}
+
+// getCRDPathConfig gets pathConfigs for one CRD recursively
+func getCRDPathConfig(types map[string]common.OpenAPIDefinition, atype string, crd string,


crd string should be changed to crd GroupVersionKind.
This can be done if the getCRDs method returns a slice of GVK.

If we can return a map[string]GroupVersionKind from getCRDs, we can use it here.

mengqiy · 2018-06-15T07:02:25Z

pkg/crds/crds.go

+			configs.addAnnotationPathConfig(
+				transformers.PathConfig{
+					CreateIfNotPresent: false,
+					GroupVersionKind:   &schema.GroupVersionKind{Kind: crdname},


Only using the kind name can collide with another CRD.
If you have changed the signature of this method, you can use the passed-in crd GroupVersionKind.

mengqiy · 2018-06-15T07:05:10Z

pkg/crds/crds.go

+
+	for typename, t := range types {
+		properties := t.Schema.SchemaProps.Properties
+		if _, ok := properties["kind"]; ok {


Readability:

_, foundKind := properties["kind"] _, foundAPIVersion := properties["apiVersion"] _, foundMetadata := properties["metadata"] if foundKind && foundAPIVersion && foundMetadata { crds = append(crds, typename) }

will fix it.

mengqiy · 2018-06-15T07:10:55Z

pkg/crds/crds.go

+
+	for typename, t := range types {
+		properties := t.Schema.SchemaProps.Properties
+		if _, ok := properties["kind"]; ok {


IIRC "kind" in properties doesn't have the same meaning as in k8s.
We should think more carefully about how to handle GVK here.
Let's discuss this tomorrow morning.

mengqiy

/lgtm
Please squash unnecessary commit(s) :)

implement CRD support and add unit tests Add integration test for crd support address comments

k8s-ci-robot · 2018-06-19T18:46:33Z

New changes are detected. LGTM label has been removed.

Liujingfang1 · 2018-06-19T19:02:40Z

squashed commits

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 12, 2018

k8s-ci-robot requested review from droot and justinsb June 12, 2018 22:36

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 12, 2018

Liujingfang1 requested review from monopole and removed request for justinsb and droot June 12, 2018 22:36

Liujingfang1 force-pushed the crdsupport branch 4 times, most recently from bcba4ff to 5eacaaa Compare June 13, 2018 16:58

Liujingfang1 requested a review from mengqiy June 13, 2018 16:59

Liujingfang1 changed the title ~~WIP: Add support for CRDs~~ Add support for CRDs Jun 13, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 13, 2018

Liujingfang1 force-pushed the crdsupport branch from 5eacaaa to c796716 Compare June 13, 2018 17:36

Liujingfang1 mentioned this pull request Jun 13, 2018

Add nameref for openshift routes? #97

Closed

Liujingfang1 removed the request for review from monopole June 13, 2018 19:53

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 13, 2018

mengqiy reviewed Jun 14, 2018

View reviewed changes

Liujingfang1 force-pushed the crdsupport branch 3 times, most recently from e5d617a to 81f9b03 Compare June 14, 2018 18:15

Liujingfang1 force-pushed the crdsupport branch 3 times, most recently from 1168aeb to 0bdba52 Compare June 14, 2018 20:08

Liujingfang1 requested a review from droot June 14, 2018 20:21

mengqiy reviewed Jun 15, 2018

View reviewed changes

Liujingfang1 force-pushed the crdsupport branch 3 times, most recently from 398f9bc to 2d7ef05 Compare June 18, 2018 18:28

Update vendor

e7ecceb

Liujingfang1 force-pushed the crdsupport branch from 2d7ef05 to 88cb945 Compare June 19, 2018 16:49

mengqiy approved these changes Jun 19, 2018

View reviewed changes

k8s-ci-robot assigned mengqiy Jun 19, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 19, 2018

Update pathconfigs library

ea00134

implement CRD support and add unit tests Add integration test for crd support address comments

Liujingfang1 force-pushed the crdsupport branch from 88cb945 to ea00134 Compare June 19, 2018 18:46

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 19, 2018

Liujingfang1 merged commit d865300 into kubernetes-sigs:master Jun 19, 2018

Liujingfang1 deleted the crdsupport branch August 10, 2018 18:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for CRDs #105

Add support for CRDs #105

Liujingfang1 commented Jun 12, 2018 •

edited

Loading

mengqiy left a comment

mengqiy Jun 14, 2018

Liujingfang1 Jun 14, 2018

mengqiy Jun 14, 2018

Liujingfang1 Jun 14, 2018

mengqiy Jun 14, 2018

Liujingfang1 commented Jun 14, 2018

mengqiy left a comment

mengqiy Jun 15, 2018

Liujingfang1 Jun 15, 2018

mengqiy Jun 15, 2018

Liujingfang1 Jun 15, 2018

mengqiy Jun 15, 2018

mengqiy Jun 15, 2018

Liujingfang1 Jun 15, 2018

mengqiy Jun 15, 2018

mengqiy left a comment

k8s-ci-robot commented Jun 19, 2018

Liujingfang1 commented Jun 19, 2018

Add support for CRDs #105

Add support for CRDs #105

Conversation

Liujingfang1 commented Jun 12, 2018 • edited Loading

mengqiy 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

Liujingfang1 commented Jun 14, 2018

mengqiy 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

mengqiy left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 19, 2018

Liujingfang1 commented Jun 19, 2018

Liujingfang1 commented Jun 12, 2018 •

edited

Loading