Support installing CRDs from yaml files in test package #51

pwittrock · 2018-06-19T18:23:34Z

also rename test package to envtest

fanzhangio

/lgtm

fanzhangio · 2018-06-19T18:39:46Z

pkg/envtest/crd.go

+	}
+
+	// White list the file extensions that may contain CRDs
+	crdExts := sets.NewString(".json", ".yaml")


Should .yml extension also be supported ?

fanzhangio · 2018-06-19T18:57:37Z

pkg/envtest/crd.go

+		}
+
+		// Check that it is actually a CRD
+		if crd.Spec.Names.Kind == "" || crd.Spec.Group == "" {


Should takeSpec.Scope to consideration as well ?

I don't thin so. This is really just to check if it was an unrelated type that happened to parse since go doesn't throw an error if the file is valid yaml but the schema doesn't match.

droot

Looks good, minor comments.

droot · 2018-06-19T22:27:04Z

pkg/envtest/crd.go

+)
+
+// CRDOptions are the options for installing CRDs
+type CRDOptions struct {


CRDInstallOptions ? (then it will be self-documenting I guess and we won't need comments)

droot · 2018-06-19T22:28:45Z

pkg/envtest/crd.go

+	// Read the CRD yamls into options.CRDs
+	if err := readCRDFiles(&options); err != nil {
+		return options.CRDs, err
+	}


why are we not returning nil (slice of CRDs) in case of error ?

droot · 2018-06-19T22:28:56Z

pkg/envtest/crd.go

+
+	// Create the CRDs in the apiserver
+	if err := CreateCRDs(config, options.CRDs); err != nil {
+		return options.CRDs, err


same question as above.

droot · 2018-06-19T22:29:06Z

pkg/envtest/crd.go

+
+	// Wait for the CRDs to appear as Resources in the apiserver
+	if err := WaitForCRDs(config, options.CRDs, options); err != nil {
+		return options.CRDs, err


same question as above.

droot · 2018-06-19T22:30:16Z

pkg/envtest/crd.go

+			if err != nil {
+				return err
+			}
+			options.CRDs = append(options.CRDs, new...)


I get it now why we are returning non-nil list in case of error above. Ignore those comments.

droot · 2018-06-19T22:31:17Z

pkg/envtest/crd.go

+
+// readCRDFiles reads the directories of CRDs in options.Paths and adds the CRD structs to options.CRDs
+func readCRDFiles(options *CRDOptions) error {
+	if len(options.Paths) > 0 {


This line is redundant. wondering if metalinter will catch this ?

I cleaned this up a bit to get the linter to pass and the tests working . PTAL

droot · 2018-06-19T22:40:07Z

pkg/envtest/crd.go

+	}
+	// Poll until all resources are found
+	return wait.PollImmediate(options.pollInterval, options.maxTime, func() (bool, error) {
+		// Create a new clientset to avoid any client caching of discovery


putting it in a separate func will improve the readability and avoid errors with sharing the waitingFor.

droot · 2018-06-19T22:44:47Z

pkg/envtest/crd.go

+			}
+
+			// Get the Resources for this GroupVersion
+			resourceList, err := cs.Discovery().ServerResourcesForGroupVersion(gv.Group + "/" + gv.Version)


unrelated: I am thinking if we should have a method to discover resources in our new client interface ? or a way to get discoveryClient without getting the cs.

Added a todo

droot · 2018-06-19T22:52:21Z

pkg/envtest/crd.go

+			// All resources found, do nothing
+			if resources.Len() == 0 {
+				delete(waitingFor, gv)
+				continue


I wonder if it is safe to delete an element from map while iterating over it ?

Yes it is. I had the same question.

https://stackoverflow.com/questions/23229975/is-it-safe-to-remove-selected-keys-from-golang-map-within-a-range-loop

droot · 2018-06-19T22:54:05Z

pkg/envtest/crd.go

+
+	// Create each CRD
+	for _, crd := range crds {
+		if _, err := cs.ApiextensionsV1beta1().CustomResourceDefinitions().Create(crd); err != nil {


wondering if we can use pkg/client do this ?

We could, but i need this for discovery anyway...

k8s-ci-robot · 2018-06-19T23:48:07Z

New changes are detected. LGTM label has been removed.

- also rename test package to envtest

pwittrock · 2018-06-19T23:49:10Z

PTAL

Support installing CRDs from yaml files in test package

Flesh out documentation on homepage

CARRY: KCP example

pwittrock assigned droot, lichuqiang and fanzhangio Jun 19, 2018

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 19, 2018

fanzhangio reviewed Jun 19, 2018

View reviewed changes

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

droot reviewed Jun 19, 2018

View reviewed changes

pwittrock force-pushed the crd-test branch from 64ae3eb to 344e6c8 Compare June 19, 2018 23:48

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

Support installing CRDs from yaml files in test package

430142c

- also rename test package to envtest

pwittrock force-pushed the crd-test branch from 344e6c8 to 430142c Compare June 19, 2018 23:48

droot approved these changes Jun 19, 2018

View reviewed changes

droot added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jun 19, 2018

k8s-ci-robot merged commit a5ba2dc into kubernetes-sigs:master Jun 19, 2018

justinsb pushed a commit to justinsb/controller-runtime that referenced this pull request Dec 7, 2018

Merge pull request kubernetes-sigs#51 from pwittrock/crd-test

2133a73

Support installing CRDs from yaml files in test package

DirectXMan12 pushed a commit that referenced this pull request Jan 31, 2020

Merge pull request #51 from pwittrock/docs

12f1fba

Flesh out documentation on homepage

sttts pushed a commit to sttts/controller-runtime that referenced this pull request Apr 26, 2024

Merge pull request kubernetes-sigs#51 from mjudeikis/kcp-0.17.move.tests

d9d2080

CARRY: KCP example

Support installing CRDs from yaml files in test package #51

Support installing CRDs from yaml files in test package #51

Uh oh!

Conversation

pwittrock commented Jun 19, 2018

Uh oh!

fanzhangio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

droot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Jun 19, 2018

Uh oh!

pwittrock commented Jun 19, 2018

Uh oh!

Uh oh!