From 5f33a4120e80c42f30e713fd1eaf151ffeba7faa Mon Sep 17 00:00:00 2001 From: dtfranz Date: Tue, 12 Sep 2023 10:40:22 -0700 Subject: [PATCH] Remove provides+required GVK and modify/remove associated tests Signed-off-by: dtfranz --- .../filter/bundle_predicates.go | 16 ---- .../filter/bundle_predicates_test.go | 28 ------- internal/catalogmetadata/types.go | 68 ++++++++++------ internal/catalogmetadata/types_test.go | 78 ------------------- .../bundles_and_dependencies.go | 21 ----- .../bundles_and_dependencies_test.go | 24 ++---- .../variablesources/crd_constraints.go | 23 ------ .../variablesources/crd_constraints_test.go | 11 ++- 8 files changed, 57 insertions(+), 212 deletions(-) diff --git a/internal/catalogmetadata/filter/bundle_predicates.go b/internal/catalogmetadata/filter/bundle_predicates.go index 9260d4c6c..e62df9187 100644 --- a/internal/catalogmetadata/filter/bundle_predicates.go +++ b/internal/catalogmetadata/filter/bundle_predicates.go @@ -52,22 +52,6 @@ func InChannel(channelName string) Predicate[catalogmetadata.Bundle] { } } -func ProvidesGVK(gvk *catalogmetadata.GVK) Predicate[catalogmetadata.Bundle] { - return func(bundle *catalogmetadata.Bundle) bool { - providedGVKs, err := bundle.ProvidedGVKs() - if err != nil { - return false - } - for i := 0; i < len(providedGVKs); i++ { - providedGVK := &providedGVKs[i] - if providedGVK.String() == gvk.String() { - return true - } - } - return false - } -} - func WithBundleImage(bundleImage string) Predicate[catalogmetadata.Bundle] { return func(bundle *catalogmetadata.Bundle) bool { return bundle.Image == bundleImage diff --git a/internal/catalogmetadata/filter/bundle_predicates_test.go b/internal/catalogmetadata/filter/bundle_predicates_test.go index 4be55aa88..95e9482f0 100644 --- a/internal/catalogmetadata/filter/bundle_predicates_test.go +++ b/internal/catalogmetadata/filter/bundle_predicates_test.go @@ -114,34 +114,6 @@ func TestInChannel(t *testing.T) { assert.False(t, f(b3)) } -func TestProvidesGVK(t *testing.T) { - b1 := &catalogmetadata.Bundle{Bundle: declcfg.Bundle{ - Properties: []property.Property{ - { - Type: property.TypeGVK, - Value: json.RawMessage(`[{"group":"foo.io","kind":"Foo","version":"v1"},{"group":"bar.io","kind":"Bar","version":"v1"}]`), - }, - }, - }} - b2 := &catalogmetadata.Bundle{Bundle: declcfg.Bundle{}} - f1 := filter.ProvidesGVK(&catalogmetadata.GVK{ - Group: "foo.io", - Version: "v1", - Kind: "Foo", - }) - f2 := filter.ProvidesGVK(&catalogmetadata.GVK{ - Group: "baz.io", - Version: "v1alpha1", - Kind: "Baz", - }) - // Filter with Bundle which provides the GVK should return true - assert.True(t, f1(b1)) - // Filter with Bundle which does not provide the GVK should return false - assert.False(t, f2(b1)) - // Filter with Bundle with no GVK should return false - assert.False(t, f1(b2)) -} - func TestWithBundleImage(t *testing.T) { b1 := &catalogmetadata.Bundle{Bundle: declcfg.Bundle{Image: "fake-image-uri-1"}} b2 := &catalogmetadata.Bundle{Bundle: declcfg.Bundle{Image: "fake-image-uri-2"}} diff --git a/internal/catalogmetadata/types.go b/internal/catalogmetadata/types.go index 624ab4eec..d58335f6d 100644 --- a/internal/catalogmetadata/types.go +++ b/internal/catalogmetadata/types.go @@ -11,6 +11,12 @@ import ( "github.com/operator-framework/operator-registry/alpha/property" ) +const ( + MediaTypePlain = "plain+v0" + MediaTypeRegistry = "registry+v1" + PropertyBundleMediaType = "olm.bundle.mediatype" +) + type Schemas interface { Package | Bundle | Channel } @@ -39,6 +45,11 @@ func (g GVKRequired) AsGVK() GVK { return GVK(g) } +type PackageRequired struct { + property.PackageRequired + SemverRange *bsemver.Range `json:"-"` +} + type Bundle struct { declcfg.Bundle CatalogName string @@ -46,11 +57,11 @@ type Bundle struct { mu sync.RWMutex // these properties are lazy loaded as they are requested - propertiesMap map[string]property.Property - bundlePackage *property.Package - semVersion *bsemver.Version - providedGVKs []GVK - requiredGVKs []GVKRequired + propertiesMap map[string]property.Property + bundlePackage *property.Package + semVersion *bsemver.Version + requiredPackages []PackageRequired + mediaType string } func (b *Bundle) Version() (*bsemver.Version, error) { @@ -60,18 +71,19 @@ func (b *Bundle) Version() (*bsemver.Version, error) { return b.semVersion, nil } -func (b *Bundle) ProvidedGVKs() ([]GVK, error) { - if err := b.loadProvidedGVKs(); err != nil { +func (b *Bundle) RequiredPackages() ([]PackageRequired, error) { + if err := b.loadRequiredPackages(); err != nil { return nil, err } - return b.providedGVKs, nil + return b.requiredPackages, nil } -func (b *Bundle) RequiredGVKs() ([]GVKRequired, error) { - if err := b.loadRequiredGVKs(); err != nil { - return nil, err +func (b *Bundle) MediaType() (string, error) { + if err := b.loadMediaType(); err != nil { + return "", err } - return b.requiredGVKs, nil + + return b.mediaType, nil } func (b *Bundle) loadPackage() error { @@ -94,28 +106,40 @@ func (b *Bundle) loadPackage() error { return nil } -func (b *Bundle) loadProvidedGVKs() error { +func (b *Bundle) loadRequiredPackages() error { b.mu.Lock() defer b.mu.Unlock() - if b.providedGVKs == nil { - providedGVKs, err := loadFromProps[[]GVK](b, property.TypeGVK, false) + if b.requiredPackages == nil { + requiredPackages, err := loadFromProps[[]PackageRequired](b, property.TypePackageRequired, false) if err != nil { - return fmt.Errorf("error determining provided GVKs for bundle %q: %s", b.Name, err) + return fmt.Errorf("error determining bundle required packages for bundle %q: %s", b.Name, err) + } + for _, requiredPackage := range requiredPackages { + semverRange, err := bsemver.ParseRange(requiredPackage.VersionRange) + if err != nil { + return fmt.Errorf( + "error parsing bundle required package semver range for bundle %q (required package %q): %s", + b.Name, + requiredPackage.PackageName, + err, + ) + } + requiredPackage.SemverRange = &semverRange } - b.providedGVKs = providedGVKs + b.requiredPackages = requiredPackages } return nil } -func (b *Bundle) loadRequiredGVKs() error { +func (b *Bundle) loadMediaType() error { b.mu.Lock() defer b.mu.Unlock() - if b.requiredGVKs == nil { - requiredGVKs, err := loadFromProps[[]GVKRequired](b, property.TypeGVKRequired, false) + if b.mediaType == "" { + mediaType, err := loadFromProps[string](b, PropertyBundleMediaType, false) if err != nil { - return fmt.Errorf("error determining required GVKs for bundle %q: %s", b.Name, err) + return fmt.Errorf("error determining bundle mediatype for bundle %q: %s", b.Name, err) } - b.requiredGVKs = requiredGVKs + b.mediaType = mediaType } return nil } diff --git a/internal/catalogmetadata/types_test.go b/internal/catalogmetadata/types_test.go index 9c6ea631c..b930802e1 100644 --- a/internal/catalogmetadata/types_test.go +++ b/internal/catalogmetadata/types_test.go @@ -71,82 +71,4 @@ func TestBundle(t *testing.T) { assert.EqualError(t, err, "bundle property with type \"olm.package\" not found") assert.Nil(t, ver) }) - - t.Run("ProvidedGVKs", func(t *testing.T) { - validGVK := &catalogmetadata.Bundle{Bundle: declcfg.Bundle{ - Name: "fake-bundle.v1", - Properties: []property.Property{ - { - Type: property.TypeGVK, - Value: json.RawMessage(`[{"group":"foo.io","kind":"Foo","version":"v1"},{"group":"bar.io","kind":"Bar","version":"v1alpha1"}]`), - }, - }, - }} - invalidGVK := &catalogmetadata.Bundle{Bundle: declcfg.Bundle{ - Name: "fake-bundle.invalid", - Properties: []property.Property{ - { - Type: property.TypeGVK, - Value: json.RawMessage(`badGvkStructure`), - }, - }, - }} - noGVK := &catalogmetadata.Bundle{Bundle: declcfg.Bundle{ - Name: "fake-bundle.noGVK", - }} - - gvk, err := validGVK.ProvidedGVKs() - assert.NoError(t, err) - assert.Equal(t, []catalogmetadata.GVK{ - {Group: "foo.io", Kind: "Foo", Version: "v1"}, - {Group: "bar.io", Kind: "Bar", Version: "v1alpha1"}, - }, gvk) - - gvk, err = invalidGVK.ProvidedGVKs() - assert.EqualError(t, err, "error determining provided GVKs for bundle \"fake-bundle.invalid\": property \"olm.gvk\" with value \"badGvkStructure\" could not be parsed: invalid character 'b' looking for beginning of value") - assert.Nil(t, gvk) - - gvk, err = noGVK.ProvidedGVKs() - assert.NoError(t, err) - assert.Nil(t, gvk) - }) - - t.Run("RequiredGVKs", func(t *testing.T) { - validGVK := &catalogmetadata.Bundle{Bundle: declcfg.Bundle{ - Name: "fake-bundle.v1", - Properties: []property.Property{ - { - Type: property.TypeGVKRequired, - Value: json.RawMessage(`[{"group":"foo.io","kind":"Foo","version":"v1"},{"group":"bar.io","kind":"Bar","version":"v1alpha1"}]`), - }, - }, - }} - invalidGVK := &catalogmetadata.Bundle{Bundle: declcfg.Bundle{ - Name: "fake-bundle.invalid", - Properties: []property.Property{ - { - Type: property.TypeGVKRequired, - Value: json.RawMessage(`badGvkStructure`), - }, - }, - }} - noGVK := &catalogmetadata.Bundle{Bundle: declcfg.Bundle{ - Name: "fake-bundle.noGVK", - }} - - gvk, err := validGVK.RequiredGVKs() - assert.NoError(t, err) - assert.Equal(t, []catalogmetadata.GVKRequired{ - {Group: "foo.io", Kind: "Foo", Version: "v1"}, - {Group: "bar.io", Kind: "Bar", Version: "v1alpha1"}, - }, gvk) - - gvk, err = invalidGVK.RequiredGVKs() - assert.EqualError(t, err, "error determining required GVKs for bundle \"fake-bundle.invalid\": property \"olm.gvk.required\" with value \"badGvkStructure\" could not be parsed: invalid character 'b' looking for beginning of value") - assert.Nil(t, gvk) - - gvk, err = noGVK.RequiredGVKs() - assert.NoError(t, err) - assert.Nil(t, gvk) - }) } diff --git a/internal/resolution/variablesources/bundles_and_dependencies.go b/internal/resolution/variablesources/bundles_and_dependencies.go index 1f0ea8835..da3a12683 100644 --- a/internal/resolution/variablesources/bundles_and_dependencies.go +++ b/internal/resolution/variablesources/bundles_and_dependencies.go @@ -107,27 +107,6 @@ func (b *BundlesAndDepsVariableSource) getEntityDependencies(ctx context.Context } } - // gather required gvk dependencies - // todo(perdasilva): disambiguate between not found and actual errors - gvkDependencies, _ := bundleEntity.RequiredGVKs() - for i := 0; i < len(gvkDependencies); i++ { - providedGvk := gvkDependencies[i].AsGVK() - gvkDependencyBundles, err := entitySource.Filter(ctx, predicates.ProvidesGVK(&providedGvk)) - if err != nil { - return nil, err - } - if len(gvkDependencyBundles) == 0 { - return nil, fmt.Errorf("could not find gvk dependencies for bundle '%s'", bundleEntity.ID) - } - for i := 0; i < len(gvkDependencyBundles); i++ { - entity := gvkDependencyBundles[i] - if _, ok := added[entity.ID]; !ok { - dependencies = append(dependencies, olmentity.NewBundleEntity(&entity)) - added[entity.ID] = struct{}{} - } - } - } - // sort bundles in version order sort.SliceStable(dependencies, func(i, j int) bool { return entitysort.ByChannelAndVersion(dependencies[i].Entity, dependencies[j].Entity) diff --git a/internal/resolution/variablesources/bundles_and_dependencies_test.go b/internal/resolution/variablesources/bundles_and_dependencies_test.go index 472369893..0218851c2 100644 --- a/internal/resolution/variablesources/bundles_and_dependencies_test.go +++ b/internal/resolution/variablesources/bundles_and_dependencies_test.go @@ -160,28 +160,16 @@ var _ = Describe("BundlesAndDepsVariableSource", func() { bundleVariables = append(bundleVariables, v) } } - Expect(bundleVariables).To(WithTransform(CollectBundleVariableIDs, Equal([]string{"bundle-2", "bundle-1", "bundle-15", "bundle-16", "bundle-17", "bundle-9", "bundle-8", "bundle-7", "bundle-5", "bundle-4", "bundle-11", "bundle-10"}))) + // Note: When accounting for Required GVKs (currently not in use), we would expect additional + // dependencies (bundles 7, 8, 9, 10, 11) to appear here due to their GVKs being required by + // some of the packages. + Expect(bundleVariables).To(WithTransform(CollectBundleVariableIDs, Equal([]string{"bundle-2", "bundle-1", "bundle-15", "bundle-16", "bundle-17", "bundle-5", "bundle-4"}))) // check dependencies for one of the bundles bundle2 := VariableWithID("bundle-2")(bundleVariables) + // Note: As above, bundle-2 has GVK requirements satisifed by bundles 7, 8, and 9, but they + // will not appear in this list as we are not currently taking Required GVKs into account Expect(bundle2.Dependencies()).To(WithTransform(CollectDeppyEntities, Equal([]*input.Entity{ - input.NewEntity("bundle-9", map[string]string{ - property.TypePackage: `{"packageName": "another-package", "version": "1.0.0"}`, - property.TypeChannel: `{"channelName":"stable","priority":0}`, - property.TypeGVK: `[{"group":"foo.io","kind":"Foo","version":"v1"}]`, - }), - input.NewEntity("bundle-8", map[string]string{ - property.TypePackage: `{"packageName": "some-other-package", "version": "1.5.0"}`, - property.TypeChannel: `{"channelName":"stable","priority":0}`, - property.TypeGVK: `[{"group":"foo.io","kind":"Foo","version":"v1"}]`, - property.TypeGVKRequired: `[{"group":"bar.io","kind":"Bar","version":"v1"}]`, - property.TypePackageRequired: `[{"packageName": "another-package", "versionRange": "< 2.0.0"}]`, - }), - input.NewEntity("bundle-7", map[string]string{ - property.TypePackage: `{"packageName": "some-other-package", "version": "1.0.0"}`, - property.TypeChannel: `{"channelName":"stable","priority":0}`, - property.TypeGVK: `[{"group":"foo.io","kind":"Foo","version":"v1"}]`, - }), input.NewEntity("bundle-5", map[string]string{ property.TypePackage: `{"packageName": "some-package", "version": "1.5.0"}`, property.TypeChannel: `{"channelName":"stable","priority":0}`, diff --git a/internal/resolution/variablesources/crd_constraints.go b/internal/resolution/variablesources/crd_constraints.go index 798fca211..43e274ab2 100644 --- a/internal/resolution/variablesources/crd_constraints.go +++ b/internal/resolution/variablesources/crd_constraints.go @@ -44,7 +44,6 @@ func (g *CRDUniquenessConstraintsVariableSource) GetVariables(ctx context.Contex // not all packages will necessarily export a CRD pkgToBundleMap := map[string]map[deppy.Identifier]struct{}{} - gvkToBundleMap := map[string]map[deppy.Identifier]struct{}{} for _, variable := range variables { switch v := variable.(type) { case *olmvariables.BundleVariable: @@ -61,19 +60,6 @@ func (g *CRDUniquenessConstraintsVariableSource) GetVariables(ctx context.Contex pkgToBundleMap[packageName] = map[deppy.Identifier]struct{}{} } pkgToBundleMap[packageName][bundleEntity.ID] = struct{}{} - - // get bundleID gvks and update map - exportedGVKs, err := bundleEntity.ProvidedGVKs() - if err != nil { - return nil, fmt.Errorf("error creating global constraints: %w", err) - } - for i := 0; i < len(exportedGVKs); i++ { - gvk := exportedGVKs[i].String() - if _, ok := gvkToBundleMap[gvk]; !ok { - gvkToBundleMap[gvk] = map[deppy.Identifier]struct{}{} - } - gvkToBundleMap[gvk][bundleEntity.ID] = struct{}{} - } } } } @@ -88,14 +74,5 @@ func (g *CRDUniquenessConstraintsVariableSource) GetVariables(ctx context.Contex variables = append(variables, olmvariables.NewBundleUniquenessVariable(varID, bundleIDs...)) } - for gvk, bundleIDMap := range gvkToBundleMap { - var bundleIDs []deppy.Identifier - for bundleID := range bundleIDMap { - bundleIDs = append(bundleIDs, bundleID) - } - varID := deppy.IdentifierFromString(fmt.Sprintf("%s gvk uniqueness", gvk)) - variables = append(variables, olmvariables.NewBundleUniquenessVariable(varID, bundleIDs...)) - } - return variables, nil } diff --git a/internal/resolution/variablesources/crd_constraints_test.go b/internal/resolution/variablesources/crd_constraints_test.go index bb456eae0..d88489666 100644 --- a/internal/resolution/variablesources/crd_constraints_test.go +++ b/internal/resolution/variablesources/crd_constraints_test.go @@ -210,7 +210,9 @@ var _ = Describe("CRDUniquenessConstraintsVariableSource", func() { } variables, err := crdConstraintVariableSource.GetVariables(ctx, entitySource) Expect(err).ToNot(HaveOccurred()) - Expect(variables).To(HaveLen(26)) + // Note: When accounting for GVK Uniqueness (which we are currently not doing), we + // would expect to have 26 variables from the 5 unique GVKs (Bar, Bit, Buz, Fiz, Foo). + Expect(variables).To(HaveLen(21)) var crdConstraintVariables []*olmvariables.BundleUniquenessVariable for _, variable := range variables { switch v := variable.(type) { @@ -218,6 +220,8 @@ var _ = Describe("CRDUniquenessConstraintsVariableSource", func() { crdConstraintVariables = append(crdConstraintVariables, v) } } + // Note: As above, the 5 GVKs would appear here as GVK uniqueness constraints + // if GVK Uniqueness were being accounted for. Expect(crdConstraintVariables).To(WithTransform(CollectGlobalConstraintVariableIDs, ConsistOf([]string{ "another-package package uniqueness", "bar-package package uniqueness", @@ -225,11 +229,6 @@ var _ = Describe("CRDUniquenessConstraintsVariableSource", func() { "test-package package uniqueness", "some-package package uniqueness", "some-other-package package uniqueness", - `group:"buz.io" version:"v1" kind:"Buz" gvk uniqueness`, - `group:"bit.io" version:"v1" kind:"Bit" gvk uniqueness`, - `group:"fiz.io" version:"v1" kind:"Fiz" gvk uniqueness`, - `group:"foo.io" version:"v1" kind:"Foo" gvk uniqueness`, - `group:"bar.io" version:"v1" kind:"Bar" gvk uniqueness`, }))) })