Remove registry dependency #35

kevinrizza · 2020-05-13T14:45:25Z

No description provided.

estroz

On first pass, looks good. Some questions/nits.

estroz · 2020-05-13T17:44:25Z

pkg/manifests/bundle.go

+	Channels       []string
+	DefaultChannel string
+	BundleImage    string
+	Csv            *operatorsv1alpha1.ClusterServiceVersion


Nit:

Suggested change

Csv *operatorsv1alpha1.ClusterServiceVersion

CSV *operatorsv1alpha1.ClusterServiceVersion

estroz · 2020-05-13T17:44:38Z

pkg/manifests/bundle.go

+	DefaultChannel string
+	BundleImage    string
+	Csv            *operatorsv1alpha1.ClusterServiceVersion
+	V1beta1crds    []*apiextensionsv1beta1.CustomResourceDefinition


Suggested change

V1beta1crds []*apiextensionsv1beta1.CustomResourceDefinition

V1beta1CRDs []*apiextensionsv1beta1.CustomResourceDefinition

estroz · 2020-05-13T17:44:48Z

pkg/manifests/bundle.go

+	BundleImage    string
+	Csv            *operatorsv1alpha1.ClusterServiceVersion
+	V1beta1crds    []*apiextensionsv1beta1.CustomResourceDefinition
+	V1crds         []*apiextensionsv1.CustomResourceDefinition


Suggested change

V1crds []*apiextensionsv1.CustomResourceDefinition

V1CRDs []*apiextensionsv1.CustomResourceDefinition

estroz · 2020-05-13T18:02:00Z

pkg/manifests/loader.go

+	}
+
+	decoder := yaml.NewYAMLOrJSONDecoder(fileReader, 30)
+	csv := unstructured.Unstructured{}


There could be more than one manifest per file.

While this is a totally fair point, and one we should address, currently the registry doesn't parse multi manifest yaml files either. IMO if we want to allow them we should update the registry first so that we aren't allowing things that won't work on the unpacking side.

estroz · 2020-05-13T18:05:33Z

pkg/validation/test_data/invalid_bundle/etcdbackups.etcd.database.coreos.com.crd.yaml

@@ -0,0 +1,13 @@
+apiVersion: apiextensions.k8s.io/v1beta1


Can you rename pkg/validation/test_data to pkg/validation/testdata?

estroz · 2020-05-13T18:06:55Z

pkg/manifests/types.go

@@ -0,0 +1,37 @@
+package manifests


Perhaps this makes more sense in a metadata package?

ecordell · 2020-05-13T16:16:34Z

pkg/manifests/loader.go

+		return utilerrors.NewAggregate(errs)
+	}
+
+	/*


Is this going away because it's still WIP?

Or is it going away because we now treat the union of CRDs in the bundle and spec.customresourcedefinitions as "provided", so this is less useful?

I think it is still valid as a validation of the CSV itself to say "Hey you said you provided these APIs, is that actually the case in your bundle?"

My intention was actually that this check should be defined as part of the validation. It seems very weird to do this specifically for this subset when many of the other bundle validations are implemented in the validation package. It only happened initially that way, I assume, because this code was originally embedded into the registry serialization code.

Commenting it out was supposed to remind me to do that, but it seems like I just overlooked it. I'll update this to reimplement it.

estroz · 2020-05-13T18:57:28Z

pkg/manifests/directory.go

-// GetManifestsDir parses all bundles and a package manifest from dir, which
-// are returned if found along with any errors or warnings encountered while
-// parsing/validating found manifests.
-func GetManifestsDir(dir string) (registry.PackageManifest, []*registry.Bundle, []errors.ManifestResult) {


This is still something the SDK needs for run packagemanifests. Will a function that returns a PackageManifest and []Bundle exist somewhere else?

Worst case the SDK can implement it, but seeing as the package manifests format will be supported indefinitely and GetBundleFromDir is implemented here, this should probably stick around right?

estroz · 2020-05-13T19:00:35Z

pkg/validation/validation.go

 )

-// PackageManifestValidator implements Validator to validate package manifests.
-var PackageManifestValidator = internal.PackageManifestValidator


I assume this will be moved to operator-registry, like the bundle validator was? The SDK uses this validator but not the PackageUpdateGraphValidator.

estroz

/lgtm

estroz · 2020-05-21T00:06:54Z

pkg/manifests/bundleloader.go

+
+	var errs []error
+	bundle := &Bundle{
+		Name: csvName,


Quick q: would this make more sense as a plain semver string?

In the future, definitely, especially given the future where a csv is no longer required in future bundle versions. For backwards compatibility though, we need to use this as the unique identifier because version itself is an optional field.

estroz · 2020-05-21T00:55:28Z

pkg/manifests/bundleloader.go

+				return nil, fmt.Errorf("unsupported CRD version %s for %s", version, f.Name())
+			}
+		default:
+			bundle.Objects = append(bundle.Objects, obj)


One difference I found between this implementation and that in OR: all objects in a bundle, including CSVs and CRDs, are appended to Bundle.Objects. Is it reasonable to have the same behavior here?

Ah, yes, it is totally reasonable and the correct behavior. I'll push a change to this pr

estroz · 2020-05-21T15:02:38Z

/lgtm

estroz · 2020-05-21T15:22:53Z

pkg/validation/internal/package_manifest.go

 var PackageManifestValidator interfaces.Validator = interfaces.ValidatorFunc(validatePackageManifests)

 func validatePackageManifests(objs ...interface{}) (results []errors.ManifestResult) {
+	fmt.Println("trying to validate package manifests")


@kevinrizza some leftover print statements

ouch, okay updated :)

estroz · 2020-05-21T15:23:00Z

pkg/validation/internal/package_manifest.go

 		switch v := obj.(type) {
-		case *registry.PackageManifest:
+		case *manifests.PackageManifest:
+			fmt.Println("found a package manifest in the list of objects")


* Remove all references to operator-framework/operator-registry * Update the type serialization and validation to use a new external api type for operator bundle, which now explicitly parses the public CRD and CSV types * Redefine the serialization functions to simply provide the types without explicitly running validation against them

dinhxuanvu

Looking good but one comment.

dinhxuanvu · 2020-05-21T18:04:47Z

pkg/manifests/bundleloader.go

+			version := obj.GetAPIVersion()
+			if version == apiextensionsv1beta1.SchemeGroupVersion.String() {
+				crd := apiextensionsv1beta1.CustomResourceDefinition{}
+				err := runtime.DefaultUnstructuredConverter.FromUnstructured(


This string-to-unstructured-to-crd conversion is the root cause of int64 to float64 error. So we should avoid converting unstructured to a specific type. Should only use unstructured to determine the object type and then convert string data straight to the object type like CRD. See: https://github.com/operator-framework/operator-registry/pull/334/files#diff-b754381164690e8ba263f719136845d3R320

Is this behavior defined for unstructured conversion?

I think this issue can be addressed in a follow-up.

I'm fine with fixing this at another time. Please make a note somewhere so it won't get lost.

dinhxuanvu

/lgtm

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 13, 2020

kevinrizza force-pushed the remove-registry-dep branch from a2e7b64 to 2ce8cb5 Compare May 13, 2020 14:57

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 13, 2020

kevinrizza force-pushed the remove-registry-dep branch from 2ce8cb5 to 6348456 Compare May 13, 2020 15:06

estroz requested changes May 13, 2020

View reviewed changes

ecordell reviewed May 13, 2020

View reviewed changes

estroz reviewed May 13, 2020

View reviewed changes

estroz mentioned this pull request May 14, 2020

pkg/registry: return error if no CSV object exists in a Bundle operator-framework/operator-registry#322

Closed

5 tasks

kevinrizza force-pushed the remove-registry-dep branch from 6348456 to 6175388 Compare May 20, 2020 20:06

estroz approved these changes May 20, 2020

View reviewed changes

openshift-ci-robot assigned estroz May 20, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 20, 2020

estroz reviewed May 21, 2020

View reviewed changes

kevinrizza force-pushed the remove-registry-dep branch from 6175388 to 6deb059 Compare May 21, 2020 13:50

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label May 21, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 21, 2020

estroz reviewed May 21, 2020

View reviewed changes

kevinrizza added 2 commits May 21, 2020 12:15

Remove operator-registry from vendor folder

eb083ab

kevinrizza force-pushed the remove-registry-dep branch from 6deb059 to eb083ab Compare May 21, 2020 16:16

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label May 21, 2020

dinhxuanvu requested changes May 21, 2020

View reviewed changes

dinhxuanvu approved these changes May 22, 2020

View reviewed changes

openshift-ci-robot assigned dinhxuanvu May 22, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 22, 2020

kevinrizza merged commit 197407c into operator-framework:master May 22, 2020

	Csv *operatorsv1alpha1.ClusterServiceVersion
	CSV *operatorsv1alpha1.ClusterServiceVersion

	V1beta1crds []*apiextensionsv1beta1.CustomResourceDefinition
	V1beta1CRDs []*apiextensionsv1beta1.CustomResourceDefinition

	V1crds []*apiextensionsv1.CustomResourceDefinition
	V1CRDs []*apiextensionsv1.CustomResourceDefinition

Remove registry dependency #35

Remove registry dependency #35

Uh oh!

Conversation

kevinrizza commented May 13, 2020

Uh oh!

estroz 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!

estroz May 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

estroz 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!

estroz commented May 21, 2020

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!

dinhxuanvu 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!

dinhxuanvu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

estroz May 13, 2020 •

edited

Loading