From e3bb5d5c998ca89d6b9630b2acba19d99ffbd192 Mon Sep 17 00:00:00 2001 From: Todd Short Date: Fri, 25 Aug 2023 10:25:33 -0400 Subject: [PATCH 1/3] Update Version regex to support ranges Fixes #345 Add positive and negative test cases. Signed-off-by: Todd Short --- api/v1alpha1/operator_types.go | 2 +- ...rators.operatorframework.io_operators.yaml | 2 +- internal/controllers/admission_test.go | 59 ++++++++++++++++++- 3 files changed, 60 insertions(+), 3 deletions(-) diff --git a/api/v1alpha1/operator_types.go b/api/v1alpha1/operator_types.go index ae3eb3985..b46997eb1 100644 --- a/api/v1alpha1/operator_types.go +++ b/api/v1alpha1/operator_types.go @@ -29,7 +29,7 @@ type OperatorSpec struct { PackageName string `json:"packageName"` //+kubebuilder:validation:MaxLength:=64 - //+kubebuilder:validation:Pattern=^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(-(0|[1-9]\d*|[0-9]*[a-zA-Z-][0-9a-zA-Z-]*)(\.(0|[1-9]\d*|[0-9]*[a-zA-Z-][0-9a-zA-Z-]*))*)?(\+([0-9a-zA-Z-]+(\.[0-9a-zA-Z-]+)*))?$ + //+kubebuilder:validation:Pattern=`^(\s*(=||!=|>|<|>=|=>|<=|=<|~|~>|\^)\s*(v?(0|[1-9]\d*|[x|X|\*])(\.(0|[1-9]\d*|x|X|\*]))?(\.(0|[1-9]\d*|x|X|\*))?(-([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?(\+([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?)\s*)((?:\s+|,\s*|\s*\|\|\s*)(=||!=|>|<|>=|=>|<=|=<|~|~>|\^)\s*(v?(0|[1-9]\d*|x|X|\*])(\.(0|[1-9]\d*|x|X|\*))?(\.(0|[1-9]\d*|x|X|\*]))?(-([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?(\+([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?)\s*)*$` //+kubebuilder:Optional // Version is an optional semver constraint on the package version. If not specified, the latest version available of the package will be installed. // If specified, the specific version of the package will be installed so long as it is available in any of the content sources available. diff --git a/config/crd/bases/operators.operatorframework.io_operators.yaml b/config/crd/bases/operators.operatorframework.io_operators.yaml index 95203347b..9f96e0201 100644 --- a/config/crd/bases/operators.operatorframework.io_operators.yaml +++ b/config/crd/bases/operators.operatorframework.io_operators.yaml @@ -51,7 +51,7 @@ spec: sources available. Examples: 1.2.3, 1.0.0-alpha, 1.0.0-rc.1 \n For more information on semver, please see https://semver.org/" maxLength: 64 - pattern: ^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(-(0|[1-9]\d*|[0-9]*[a-zA-Z-][0-9a-zA-Z-]*)(\.(0|[1-9]\d*|[0-9]*[a-zA-Z-][0-9a-zA-Z-]*))*)?(\+([0-9a-zA-Z-]+(\.[0-9a-zA-Z-]+)*))?$ + pattern: ^(\s*(=||!=|>|<|>=|=>|<=|=<|~|~>|\^)\s*(v?(0|[1-9]\d*|[x|X|\*])(\.(0|[1-9]\d*|x|X|\*]))?(\.(0|[1-9]\d*|x|X|\*))?(-([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?(\+([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?)\s*)((?:\s+|,\s*|\s*\|\|\s*)(=||!=|>|<|>=|=>|<=|=<|~|~>|\^)\s*(v?(0|[1-9]\d*|x|X|\*])(\.(0|[1-9]\d*|x|X|\*))?(\.(0|[1-9]\d*|x|X|\*]))?(-([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?(\+([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?)\s*)*$ type: string required: - packageName diff --git a/internal/controllers/admission_test.go b/internal/controllers/admission_test.go index ae568d019..60b106664 100644 --- a/internal/controllers/admission_test.go +++ b/internal/controllers/admission_test.go @@ -61,6 +61,16 @@ var _ = Describe("Operator Spec Validations", func() { "1.2.3-pre+bad_metadata", "1.2.-3", ".1.2.3", + "<<1.2.3", + ">>1.2.3", + ">~1.2.3", + "==1.2.3", + "=!1.2.3", + "!1.2.3", + "1.Y", + ">1.2.3 && <2.3.4", + ">1.2.3;<2.3.4", + "1.2.3 - 2.3.4", } for _, invalidSemver := range invalidSemvers { err := cl.Create(ctx, operator(operatorsv1alpha1.OperatorSpec{ @@ -69,7 +79,54 @@ var _ = Describe("Operator Spec Validations", func() { })) Expect(err).To(HaveOccurred(), "expected error for invalid semver %q", invalidSemver) - Expect(err.Error()).To(ContainSubstring("spec.version in body should match '^(0|[1-9]\\d*)\\.(0|[1-9]\\d*)\\.(0|[1-9]\\d*)(-(0|[1-9]\\d*|[0-9]*[a-zA-Z-][0-9a-zA-Z-]*)(\\.(0|[1-9]\\d*|[0-9]*[a-zA-Z-][0-9a-zA-Z-]*))*)?(\\+([0-9a-zA-Z-]+(\\.[0-9a-zA-Z-]+)*))?$'")) + // Don't need to include the whole regex, this should be enough to match the MasterMinds regex + Expect(err.Error()).To(ContainSubstring("spec.version in body should match '^(\\s*(=||!=|>|<|>=|=>|<=|=<|~|~>|\\^)")) + } + }) + It("should pass if a valid semver range given", func() { + validSemvers := []string{ + ">=1.2.3", + "=>1.2.3", + ">= 1.2.3", + ">=v1.2.3", + ">= v1.2.3", + "<=1.2.3", + "=<1.2.3", + "=1.2.3", + "!=1.2.3", + "<1.2.3", + ">1.2.3", + "~1.2.2", + "~>1.2.3", + "^1.2.3", + "1.2.3", + "v1.2.3", + "1.x", + "1.X", + "1.*", + "1.2.x", + "1.2.X", + "1.2.*", + ">=1.2.3 <2.3.4", + ">=1.2.3,<2.3.4", + ">=1.2.3, <2.3.4", + "<1.2.3||>2.3.4", + "<1.2.3|| >2.3.4", + "<1.2.3 ||>2.3.4", + "<1.2.3 || >2.3.4", + ">1.0.0,<1.2.3 || >2.1.0", + "<1.2.3-abc >2.3.4-def", + "<1.2.3-abc+def >2.3.4-ghi+jkl", + } + for _, validSemver := range validSemvers { + op := operator(operatorsv1alpha1.OperatorSpec{ + PackageName: "package", + Version: validSemver, + }) + err := cl.Create(ctx, op) + Expect(err).NotTo(HaveOccurred(), "expected success for semver range '%q': %w", validSemver, err) + err = cl.Delete(ctx, op) + Expect(err).NotTo(HaveOccurred(), "unexpected error deleting valid semver '%q': %w", validSemver, err) } }) It("should fail if an invalid channel name is given", func() { From ac07fe1b7f397cdabb84af9d6d288f69d3f8ca46 Mon Sep 17 00:00:00 2001 From: Todd Short Date: Tue, 29 Aug 2023 15:31:36 -0400 Subject: [PATCH 2/3] Introduce Masterminds/semver Fixes #346 Add support for Masterminds/semver for .spec.Version This is a bit more entangled into the code than I expected, most instances of bsemver were replaced. Signed-off-by: Todd Short --- go.mod | 1 + go.sum | 2 ++ internal/controllers/validators/validators.go | 4 +-- internal/resolution/entities/bundle_entity.go | 10 ++++++- .../resolution/entities/bundle_entity_test.go | 30 +++++++++++++++---- .../resolution/util/predicates/predicates.go | 16 ++++++++-- .../util/predicates/predicates_test.go | 29 +++++++++++++++--- internal/resolution/util/sort/sort.go | 4 +-- .../bundles_and_dependencies.go | 2 +- .../variablesources/required_package.go | 8 ++--- .../variablesources/required_package_test.go | 2 +- 11 files changed, 86 insertions(+), 22 deletions(-) diff --git a/go.mod b/go.mod index 445daa2e3..dce8fd0ce 100644 --- a/go.mod +++ b/go.mod @@ -3,6 +3,7 @@ module github.com/operator-framework/operator-controller go 1.20 require ( + github.com/Masterminds/semver/v3 v3.2.0 github.com/blang/semver/v4 v4.0.0 github.com/go-logr/logr v1.2.4 github.com/onsi/ginkgo/v2 v2.11.0 diff --git a/go.sum b/go.sum index 25b504b25..794b9c01c 100644 --- a/go.sum +++ b/go.sum @@ -48,6 +48,8 @@ github.com/Azure/go-autorest/logger v0.2.0/go.mod h1:T9E3cAhj2VqvPOtCYAvby9aBXkZ github.com/Azure/go-autorest/tracing v0.6.0/go.mod h1:+vhtPC754Xsa23ID7GlGsrdKBpUA79WCAKPPZVC2DeU= github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU= github.com/BurntSushi/xgb v0.0.0-20160522181843-27f122750802/go.mod h1:IVnqGOEym/WlBOVXweHU+Q+/VP0lqqI8lqeDx9IjBqo= +github.com/Masterminds/semver/v3 v3.2.0 h1:3MEsd0SM6jqZojhjLWWeBY+Kcjy9i6MQAeY7YgDP83g= +github.com/Masterminds/semver/v3 v3.2.0/go.mod h1:qvl/7zhW3nngYb5+80sSMF+FG2BjYrf8m9wsX0PNOMQ= github.com/Microsoft/go-winio v0.4.11/go.mod h1:VhR8bwka0BXejwEJY73c50VrPtXAaKcyvVC4A4RozmA= github.com/Microsoft/go-winio v0.4.14/go.mod h1:qXqCSQ3Xa7+6tgxaGTIe4Kpcdsi+P8jBhyzoq1bpyYA= github.com/Microsoft/go-winio v0.4.15-0.20190919025122-fc70bd9a86b5/go.mod h1:tTuCMEN+UleMWgg9dVx4Hu52b1bJo+59jBh3ajtinzw= diff --git a/internal/controllers/validators/validators.go b/internal/controllers/validators/validators.go index 3caa3f934..f71a7897d 100644 --- a/internal/controllers/validators/validators.go +++ b/internal/controllers/validators/validators.go @@ -3,7 +3,7 @@ package validators import ( "fmt" - bsemver "github.com/blang/semver/v4" + mmsemver "github.com/Masterminds/semver/v3" operatorsv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1" ) @@ -18,7 +18,7 @@ func validateSemver(operator *operatorsv1alpha1.Operator) error { if operator.Spec.Version == "" { return nil } - if _, err := bsemver.Parse(operator.Spec.Version); err != nil { + if _, err := mmsemver.NewConstraint(operator.Spec.Version); err != nil { return fmt.Errorf("invalid .spec.version: %w", err) } return nil diff --git a/internal/resolution/entities/bundle_entity.go b/internal/resolution/entities/bundle_entity.go index b9ccf4cce..9829436ef 100644 --- a/internal/resolution/entities/bundle_entity.go +++ b/internal/resolution/entities/bundle_entity.go @@ -5,6 +5,7 @@ import ( "fmt" "sync" + mmsemver "github.com/Masterminds/semver/v3" bsemver "github.com/blang/semver/v4" "github.com/operator-framework/deppy/pkg/deppy/input" "github.com/operator-framework/operator-registry/alpha/property" @@ -87,13 +88,20 @@ func (b *BundleEntity) PackageName() (string, error) { return b.bundlePackage.PackageName, nil } -func (b *BundleEntity) Version() (*bsemver.Version, error) { +func (b *BundleEntity) VersionBlang() (*bsemver.Version, error) { if err := b.loadPackage(); err != nil { return nil, err } return b.semVersion, nil } +func (b *BundleEntity) VersionMasterminds() (*mmsemver.Version, error) { + if err := b.loadPackage(); err != nil { + return nil, err + } + return mmsemver.NewVersion(b.bundlePackage.Version) +} + func (b *BundleEntity) ProvidedGVKs() ([]GVK, error) { if err := b.loadProvidedGVKs(); err != nil { return nil, err diff --git a/internal/resolution/entities/bundle_entity_test.go b/internal/resolution/entities/bundle_entity_test.go index 087575970..2f48b97f1 100644 --- a/internal/resolution/entities/bundle_entity_test.go +++ b/internal/resolution/entities/bundle_entity_test.go @@ -4,6 +4,7 @@ import ( "fmt" "testing" + mmsemver "github.com/Masterminds/semver/v3" bsemver "github.com/blang/semver/v4" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -48,19 +49,28 @@ var _ = Describe("BundleEntity", func() { }) Describe("Version", func() { - It("should return the bundle version if present", func() { + It("should return the bundle blang version if present", func() { entity := input.NewEntity("operatorhub/prometheus/0.14.0", map[string]string{ "olm.package": "{\"packageName\":\"prometheus\",\"version\":\"0.14.0\"}", }) bundleEntity := olmentity.NewBundleEntity(entity) - version, err := bundleEntity.Version() + version, err := bundleEntity.VersionBlang() Expect(err).ToNot(HaveOccurred()) Expect(*version).To(Equal(bsemver.MustParse("0.14.0"))) }) + It("should return the bundle Masterminds version if present", func() { + entity := input.NewEntity("operatorhub/prometheus/0.14.0", map[string]string{ + "olm.package": "{\"packageName\":\"prometheus\",\"version\":\"0.14.0\"}", + }) + bundleEntity := olmentity.NewBundleEntity(entity) + version, err := bundleEntity.VersionMasterminds() + Expect(err).ToNot(HaveOccurred()) + Expect(*version).To(Equal(*mmsemver.MustParse("0.14.0"))) + }) It("should return an error if the property is not found", func() { entity := input.NewEntity("operatorhub/prometheus/0.14.0", map[string]string{}) bundleEntity := olmentity.NewBundleEntity(entity) - version, err := bundleEntity.Version() + version, err := bundleEntity.VersionBlang() Expect(version).To(BeNil()) Expect(err.Error()).To(Equal("error determining package for entity 'operatorhub/prometheus/0.14.0': required property 'olm.package' not found")) }) @@ -69,7 +79,7 @@ var _ = Describe("BundleEntity", func() { "olm.package": "badPackageStructure", }) bundleEntity := olmentity.NewBundleEntity(entity) - version, err := bundleEntity.Version() + version, err := bundleEntity.VersionBlang() Expect(version).To(BeNil()) Expect(err.Error()).To(Equal("error determining package for entity 'operatorhub/prometheus/0.14.0': property 'olm.package' ('badPackageStructure') could not be parsed: invalid character 'b' looking for beginning of value")) }) @@ -78,8 +88,18 @@ var _ = Describe("BundleEntity", func() { "olm.package": "{\"packageName\":\"prometheus\",\"version\":\"badversion\"}", }) bundleEntity := olmentity.NewBundleEntity(entity) - version, err := bundleEntity.Version() + version, err := bundleEntity.VersionBlang() + Expect(version).To(BeNil()) + Expect(err.Error()).To(Equal("could not parse semver (badversion) for entity 'operatorhub/prometheus/0.14.0': No Major.Minor.Patch elements found")) + }) + It("should return error if the version is malformed", func() { + entity := input.NewEntity("operatorhub/prometheus/0.14.0", map[string]string{ + "olm.package": "{\"packageName\":\"prometheus\",\"version\":\"badversion\"}", + }) + bundleEntity := olmentity.NewBundleEntity(entity) + version, err := bundleEntity.VersionMasterminds() Expect(version).To(BeNil()) + // This is still a blang error, as it does not get to the Masterminds code Expect(err.Error()).To(Equal("could not parse semver (badversion) for entity 'operatorhub/prometheus/0.14.0': No Major.Minor.Patch elements found")) }) }) diff --git a/internal/resolution/util/predicates/predicates.go b/internal/resolution/util/predicates/predicates.go index aa351b9e8..5909a4134 100644 --- a/internal/resolution/util/predicates/predicates.go +++ b/internal/resolution/util/predicates/predicates.go @@ -1,6 +1,7 @@ package predicates import ( + mmsemver "github.com/Masterminds/semver/v3" bsemver "github.com/blang/semver/v4" "github.com/operator-framework/deppy/pkg/deppy/input" @@ -18,10 +19,21 @@ func WithPackageName(packageName string) input.Predicate { } } -func InSemverRange(semverRange bsemver.Range) input.Predicate { +func InMastermindsSemverRange(semverRange *mmsemver.Constraints) input.Predicate { return func(entity *input.Entity) bool { bundleEntity := olmentity.NewBundleEntity(entity) - bundleVersion, err := bundleEntity.Version() + bundleVersion, err := bundleEntity.VersionMasterminds() + if err != nil { + return false + } + return semverRange.Check(bundleVersion) + } +} + +func InBlangSemverRange(semverRange bsemver.Range) input.Predicate { + return func(entity *input.Entity) bool { + bundleEntity := olmentity.NewBundleEntity(entity) + bundleVersion, err := bundleEntity.VersionBlang() if err != nil { return false } diff --git a/internal/resolution/util/predicates/predicates_test.go b/internal/resolution/util/predicates/predicates_test.go index 3c9c12796..c47f0ce5d 100644 --- a/internal/resolution/util/predicates/predicates_test.go +++ b/internal/resolution/util/predicates/predicates_test.go @@ -3,6 +3,7 @@ package predicates_test import ( "testing" + mmsemver "github.com/Masterminds/semver/v3" bsemver "github.com/blang/semver/v4" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -33,20 +34,40 @@ var _ = Describe("Predicates", func() { }) }) - Describe("InSemverRange", func() { + Describe("InMastermindsSemverRange", func() { + It("should return true when the entity has the has version in the right range", func() { + entity := input.NewEntity("test", map[string]string{ + property.TypePackage: `{"packageName": "mypackage", "version": "1.0.0"}`, + }) + inRange, err := mmsemver.NewConstraint(">=1.0.0") + Expect(err).NotTo(HaveOccurred()) + notInRange, err := mmsemver.NewConstraint(">=2.0.0") + Expect(err).NotTo(HaveOccurred()) + Expect(predicates.InMastermindsSemverRange(inRange)(entity)).To(BeTrue()) + Expect(predicates.InMastermindsSemverRange(notInRange)(entity)).To(BeFalse()) + }) + It("should return false when the entity does not have a version", func() { + entity := input.NewEntity("test", map[string]string{}) + inRange, err := mmsemver.NewConstraint(">=1.0.0") + Expect(err).NotTo(HaveOccurred()) + Expect(predicates.InMastermindsSemverRange(inRange)(entity)).To(BeFalse()) + }) + }) + + Describe("InBlangSemverRange", func() { It("should return true when the entity has the has version in the right range", func() { entity := input.NewEntity("test", map[string]string{ property.TypePackage: `{"packageName": "mypackage", "version": "1.0.0"}`, }) inRange := bsemver.MustParseRange(">=1.0.0") notInRange := bsemver.MustParseRange(">=2.0.0") - Expect(predicates.InSemverRange(inRange)(entity)).To(BeTrue()) - Expect(predicates.InSemverRange(notInRange)(entity)).To(BeFalse()) + Expect(predicates.InBlangSemverRange(inRange)(entity)).To(BeTrue()) + Expect(predicates.InBlangSemverRange(notInRange)(entity)).To(BeFalse()) }) It("should return false when the entity does not have a version", func() { entity := input.NewEntity("test", map[string]string{}) inRange := bsemver.MustParseRange(">=1.0.0") - Expect(predicates.InSemverRange(inRange)(entity)).To(BeFalse()) + Expect(predicates.InBlangSemverRange(inRange)(entity)).To(BeFalse()) }) }) diff --git a/internal/resolution/util/sort/sort.go b/internal/resolution/util/sort/sort.go index d013b982f..ee0f2b42d 100644 --- a/internal/resolution/util/sort/sort.go +++ b/internal/resolution/util/sort/sort.go @@ -68,8 +68,8 @@ func channelOrder(e1, e2 *entities.BundleEntity) int { } func versionOrder(e1, e2 *entities.BundleEntity) int { - ver1, err1 := e1.Version() - ver2, err2 := e2.Version() + ver1, err1 := e1.VersionBlang() + ver2, err2 := e2.VersionBlang() errComp := compareErrors(err1, err2) if errComp != 0 { // the sign gets inverted because version is sorted diff --git a/internal/resolution/variablesources/bundles_and_dependencies.go b/internal/resolution/variablesources/bundles_and_dependencies.go index 0301487e4..1f0ea8835 100644 --- a/internal/resolution/variablesources/bundles_and_dependencies.go +++ b/internal/resolution/variablesources/bundles_and_dependencies.go @@ -91,7 +91,7 @@ func (b *BundlesAndDepsVariableSource) getEntityDependencies(ctx context.Context if err != nil { return nil, err } - packageDependencyBundles, err := entitySource.Filter(ctx, input.And(predicates.WithPackageName(requiredPackage.PackageName), predicates.InSemverRange(semverRange))) + packageDependencyBundles, err := entitySource.Filter(ctx, input.And(predicates.WithPackageName(requiredPackage.PackageName), predicates.InBlangSemverRange(semverRange))) if err != nil { return nil, err } diff --git a/internal/resolution/variablesources/required_package.go b/internal/resolution/variablesources/required_package.go index 229e34c7e..a580c4618 100644 --- a/internal/resolution/variablesources/required_package.go +++ b/internal/resolution/variablesources/required_package.go @@ -4,7 +4,7 @@ import ( "context" "fmt" - bsemver "github.com/blang/semver/v4" + mmsemver "github.com/Masterminds/semver/v3" "github.com/operator-framework/deppy/pkg/deppy" "github.com/operator-framework/deppy/pkg/deppy/input" @@ -21,14 +21,14 @@ type RequiredPackageVariableSourceOption func(*RequiredPackageVariableSource) er func InVersionRange(versionRange string) RequiredPackageVariableSourceOption { return func(r *RequiredPackageVariableSource) error { if versionRange != "" { - vr, err := bsemver.ParseRange(versionRange) + vr, err := mmsemver.NewConstraint(versionRange) if err == nil { r.versionRange = versionRange - r.predicates = append(r.predicates, predicates.InSemverRange(vr)) + r.predicates = append(r.predicates, predicates.InMastermindsSemverRange(vr)) return nil } - return fmt.Errorf("invalid version range '%s': %v", versionRange, err) + return fmt.Errorf("invalid version range '%s': %w", versionRange, err) } return nil } diff --git a/internal/resolution/variablesources/required_package_test.go b/internal/resolution/variablesources/required_package_test.go index 19c14824b..834054c70 100644 --- a/internal/resolution/variablesources/required_package_test.go +++ b/internal/resolution/variablesources/required_package_test.go @@ -80,7 +80,7 @@ var _ = Describe("RequiredPackageVariableSource", func() { It("should filter by version range", func() { // recreate source with version range option var err error - rpvs, err = variablesources.NewRequiredPackageVariableSource(packageName, variablesources.InVersionRange(">=1.0.0 !2.0.0 <3.0.0")) + rpvs, err = variablesources.NewRequiredPackageVariableSource(packageName, variablesources.InVersionRange(">=1.0.0 !=2.0.0 <3.0.0")) Expect(err).NotTo(HaveOccurred()) variables, err := rpvs.GetVariables(context.TODO(), mockEntitySource) From f005767871ef7677b9452930663da6ccf8a8af87 Mon Sep 17 00:00:00 2001 From: Todd Short Date: Thu, 31 Aug 2023 17:22:54 -0400 Subject: [PATCH 3/3] fixup! Introduce Masterminds/semver Signed-off-by: Todd Short --- internal/resolution/entities/bundle_entity.go | 10 +------- .../resolution/entities/bundle_entity_test.go | 24 +++++++------------ .../resolution/util/predicates/predicates.go | 14 ++++++++--- internal/resolution/util/sort/sort.go | 4 ++-- 4 files changed, 22 insertions(+), 30 deletions(-) diff --git a/internal/resolution/entities/bundle_entity.go b/internal/resolution/entities/bundle_entity.go index 9829436ef..b9ccf4cce 100644 --- a/internal/resolution/entities/bundle_entity.go +++ b/internal/resolution/entities/bundle_entity.go @@ -5,7 +5,6 @@ import ( "fmt" "sync" - mmsemver "github.com/Masterminds/semver/v3" bsemver "github.com/blang/semver/v4" "github.com/operator-framework/deppy/pkg/deppy/input" "github.com/operator-framework/operator-registry/alpha/property" @@ -88,20 +87,13 @@ func (b *BundleEntity) PackageName() (string, error) { return b.bundlePackage.PackageName, nil } -func (b *BundleEntity) VersionBlang() (*bsemver.Version, error) { +func (b *BundleEntity) Version() (*bsemver.Version, error) { if err := b.loadPackage(); err != nil { return nil, err } return b.semVersion, nil } -func (b *BundleEntity) VersionMasterminds() (*mmsemver.Version, error) { - if err := b.loadPackage(); err != nil { - return nil, err - } - return mmsemver.NewVersion(b.bundlePackage.Version) -} - func (b *BundleEntity) ProvidedGVKs() ([]GVK, error) { if err := b.loadProvidedGVKs(); err != nil { return nil, err diff --git a/internal/resolution/entities/bundle_entity_test.go b/internal/resolution/entities/bundle_entity_test.go index 2f48b97f1..b7b11052b 100644 --- a/internal/resolution/entities/bundle_entity_test.go +++ b/internal/resolution/entities/bundle_entity_test.go @@ -54,7 +54,7 @@ var _ = Describe("BundleEntity", func() { "olm.package": "{\"packageName\":\"prometheus\",\"version\":\"0.14.0\"}", }) bundleEntity := olmentity.NewBundleEntity(entity) - version, err := bundleEntity.VersionBlang() + version, err := bundleEntity.Version() Expect(err).ToNot(HaveOccurred()) Expect(*version).To(Equal(bsemver.MustParse("0.14.0"))) }) @@ -63,14 +63,16 @@ var _ = Describe("BundleEntity", func() { "olm.package": "{\"packageName\":\"prometheus\",\"version\":\"0.14.0\"}", }) bundleEntity := olmentity.NewBundleEntity(entity) - version, err := bundleEntity.VersionMasterminds() + bVersion, err := bundleEntity.Version() Expect(err).ToNot(HaveOccurred()) - Expect(*version).To(Equal(*mmsemver.MustParse("0.14.0"))) + mVersion, err := mmsemver.NewVersion(bVersion.String()) + Expect(err).ToNot(HaveOccurred()) + Expect(*mVersion).To(Equal(*mmsemver.MustParse("0.14.0"))) }) It("should return an error if the property is not found", func() { entity := input.NewEntity("operatorhub/prometheus/0.14.0", map[string]string{}) bundleEntity := olmentity.NewBundleEntity(entity) - version, err := bundleEntity.VersionBlang() + version, err := bundleEntity.Version() Expect(version).To(BeNil()) Expect(err.Error()).To(Equal("error determining package for entity 'operatorhub/prometheus/0.14.0': required property 'olm.package' not found")) }) @@ -79,7 +81,7 @@ var _ = Describe("BundleEntity", func() { "olm.package": "badPackageStructure", }) bundleEntity := olmentity.NewBundleEntity(entity) - version, err := bundleEntity.VersionBlang() + version, err := bundleEntity.Version() Expect(version).To(BeNil()) Expect(err.Error()).To(Equal("error determining package for entity 'operatorhub/prometheus/0.14.0': property 'olm.package' ('badPackageStructure') could not be parsed: invalid character 'b' looking for beginning of value")) }) @@ -88,18 +90,8 @@ var _ = Describe("BundleEntity", func() { "olm.package": "{\"packageName\":\"prometheus\",\"version\":\"badversion\"}", }) bundleEntity := olmentity.NewBundleEntity(entity) - version, err := bundleEntity.VersionBlang() - Expect(version).To(BeNil()) - Expect(err.Error()).To(Equal("could not parse semver (badversion) for entity 'operatorhub/prometheus/0.14.0': No Major.Minor.Patch elements found")) - }) - It("should return error if the version is malformed", func() { - entity := input.NewEntity("operatorhub/prometheus/0.14.0", map[string]string{ - "olm.package": "{\"packageName\":\"prometheus\",\"version\":\"badversion\"}", - }) - bundleEntity := olmentity.NewBundleEntity(entity) - version, err := bundleEntity.VersionMasterminds() + version, err := bundleEntity.Version() Expect(version).To(BeNil()) - // This is still a blang error, as it does not get to the Masterminds code Expect(err.Error()).To(Equal("could not parse semver (badversion) for entity 'operatorhub/prometheus/0.14.0': No Major.Minor.Patch elements found")) }) }) diff --git a/internal/resolution/util/predicates/predicates.go b/internal/resolution/util/predicates/predicates.go index 5909a4134..f374efa2c 100644 --- a/internal/resolution/util/predicates/predicates.go +++ b/internal/resolution/util/predicates/predicates.go @@ -22,18 +22,26 @@ func WithPackageName(packageName string) input.Predicate { func InMastermindsSemverRange(semverRange *mmsemver.Constraints) input.Predicate { return func(entity *input.Entity) bool { bundleEntity := olmentity.NewBundleEntity(entity) - bundleVersion, err := bundleEntity.VersionMasterminds() + bVersion, err := bundleEntity.Version() if err != nil { return false } - return semverRange.Check(bundleVersion) + // No error should occur here because the simple version was successfully parsed by blang + // We are unaware of any tests cases that would cause one to fail but not the other + // This will cause code coverage to drop for this line. We don't ignore the error because + // there might be that one extreme edge case that might cause one to fail but not the other + mVersion, err := mmsemver.NewVersion(bVersion.String()) + if err != nil { + return false + } + return semverRange.Check(mVersion) } } func InBlangSemverRange(semverRange bsemver.Range) input.Predicate { return func(entity *input.Entity) bool { bundleEntity := olmentity.NewBundleEntity(entity) - bundleVersion, err := bundleEntity.VersionBlang() + bundleVersion, err := bundleEntity.Version() if err != nil { return false } diff --git a/internal/resolution/util/sort/sort.go b/internal/resolution/util/sort/sort.go index ee0f2b42d..d013b982f 100644 --- a/internal/resolution/util/sort/sort.go +++ b/internal/resolution/util/sort/sort.go @@ -68,8 +68,8 @@ func channelOrder(e1, e2 *entities.BundleEntity) int { } func versionOrder(e1, e2 *entities.BundleEntity) int { - ver1, err1 := e1.VersionBlang() - ver2, err2 := e2.VersionBlang() + ver1, err1 := e1.Version() + ver2, err2 := e2.Version() errComp := compareErrors(err1, err2) if errComp != 0 { // the sign gets inverted because version is sorted