Introduce Masterminds/semver #374

tmshort · 2023-08-29T19:34:10Z

Description

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.

Fixes #345 and #346.

Includes the regex commit.

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

internal/resolution/variablesources/required_package_test.go

codecov · 2023-08-29T19:39:19Z

Codecov Report

Patch coverage: 85.71% and project coverage change: -0.04% ⚠️

Comparison is base (db08a62) 81.68% compared to head (f005767) 81.64%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #374      +/-   ##
==========================================
- Coverage   81.68%   81.64%   -0.04%     
==========================================
  Files          21       21              
  Lines         928      937       +9     
==========================================
+ Hits          758      765       +7     
- Misses        118      119       +1     
- Partials       52       53       +1

Flag	Coverage Δ
e2e	`61.79% <50.00%> (-0.50%)`	⬇️
unit	`77.23% <85.71%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
api/v1alpha1/operator_types.go	`100.00% <ø> (ø)`
internal/resolution/util/predicates/predicates.go	`91.83% <77.77%> (-3.17%)`	⬇️
internal/controllers/validators/validators.go	`100.00% <100.00%> (ø)`
...lution/variablesources/bundles_and_dependencies.go	`87.09% <100.00%> (ø)`
...nal/resolution/variablesources/required_package.go	`73.80% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tmshort · 2023-08-29T20:06:06Z

I think it needs more testing, but this is a start.
/hold

api/v1alpha1/operator_types.go

internal/resolution/variablesources/required_package_test.go

ncdc · 2023-08-29T20:10:24Z

@tmshort we don't have Prow on this repo; if you'd like to "hold" the PR, I'd recommend converting it to draft.

joelanford · 2023-08-29T20:43:01Z

internal/resolution/variablesources/bundles_and_dependencies.go

@@ -87,7 +87,7 @@ func (b *BundlesAndDepsVariableSource) getEntityDependencies(ctx context.Context
 	// todo(perdasilva): disambiguate between not found and actual errors
 	requiredPackages, _ := bundleEntity.RequiredPackages()
 	for _, requiredPackage := range requiredPackages {
-		semverRange, err := bsemver.ParseRange(requiredPackage.VersionRange)
+		semverRange, err := mmsemver.NewConstraint(requiredPackage.VersionRange)


Technically, this should remain blang if we don't want to change the meaning of (and possibly break) the olm.package.required constraints.

I can look at it; changes were only made if they propagated from validation.gos changes. This likely means that it would require intermixing Masterminds and blang code here.

Yeah, looks like you'll need to convert a blang version to a masterminds verison somewhere along the line.

It looks like bundles_and_dependencies.go and required_package.go both call predicates.InVersionRange, so it seems like we'll have to break that in two. Perhaps:

predicates.InMastermindsVersionRange

predicates.InBlangVersionRange

And then predicates.InMasterMindsVersionRange converts the bundle entity blang version to a masterminds version to check against the mastermind constraint?

blang.Range is a function and cannot be converted into a MasterMind.Constraints (which is a struct). I'm working out a couple different methods; none of which are pretty...

tmshort · 2023-08-31T13:25:04Z

@joelanford ready for review

grokspawn · 2023-08-31T13:48:58Z

internal/controllers/admission_test.go

+			"1.Y",
+			">1.2.3 && <2.3.4",
+			">1.2.3;<2.3.4",
+			"1.2.3 - 2.3.4",


worth including the blang '!=' equivalent here? (e.g. "!1.2.3")

tmshort · 2023-08-31T16:07:25Z

/unhold

Fixes operator-framework#345 Add positive and negative test cases. Signed-off-by: Todd Short <tshort@redhat.com>

Fixes operator-framework#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 <tshort@redhat.com>

tmshort · 2023-08-31T20:03:51Z

ping @grokspawn @joelanford this should be ready now

joelanford · 2023-08-31T20:16:01Z

internal/resolution/entities/bundle_entity.go

+	if err := b.loadPackage(); err != nil {
+		return nil, err
+	}
+	return mmsemver.NewVersion(b.bundlePackage.Version)


Nit: the asymmetry of VersionBlang() stuff being parsed during b.loadPackage() and the masterminds version being parsed here every time is maybe a bit unexpected and feels a bit off?

Other potential options:

add a new sibling field to b.semVersion, parse both during loadPackage()

parse into a blang version and then just build a masterminds.Version struct (either here or in loadPackage) using the blang version fields (this might be nice because its one fewer error to handle)

have this Version() method just return a string and expect that caller to do the parsing.

Another option: do the (option 2) conversion from blang version to masterminds version in the InMastermindsVersionRange() function.

TL;DR: this is what happens when you try to optimize code coverage!

I had created a new field, but all errors occurred during blang code processing (since it's just a simple version), and subsequently the masterminds error handling code was never executed, lowering code coverage. I could get rid of the error handling code, but that's bad form.

The Masterminds.Version struct has no user-accessible fields, so you can't create the structure that way. It actually makes more sense to do a blang.Version.String() and feed that into Masterminds.NewVersion(), but it's the same error handling problem as option 1.

This causes the version string to be parsed all the time, also non-optimal, and worse performance than option 1.

EDIT: Using just one semver library would make this moot.
EDIT 2: Option 3 is probably the cleanest in terms of API cleanliness, but not efficiency.

Another option: do the (option 2) conversion from blang version to masterminds version in the InMastermindsVersionRange() function.

AFAICT, this means passing an blang structure into a mastermind-named function, which seems bad form.

this means passing an blang structure into a mastermind-named function, which seems bad form.

I don't think so.

func InMastermindsSemverRange(semverRange *mmsemver.Constraints) input.Predicate { return func(entity *input.Entity) bool { bundleEntity := olmentity.NewBundleEntity(entity) bundleVersion, err := bundleEntity.Version() // this is a blang version if err != nil { return false } return semverRange.Check(blangToMasterminds(bundleVersion)) } }

Sorry, I was thinking of this function: https://github.com/Masterminds/semver/blob/2f39fdc11c33c38e8b8b15b1f04334ba84e751f2/version.go#L214

Although option 3 might be the cleanest, it is changing the behavior of sortVersion().

Anything I do to try to fix the nit will negatively impact code coverage.

IMO, trying to cover every error path just leads to brittle tests that make refactoring hard. I think as long as we're covering the happy paths, and the coverage "regression" is only happening because of some refactoring of when/how errors are handled, we shouldn't spend time dealing in the 1s or .1s of coverage percentages.

Is there a way to specify a (minimum, err maximum? negative) threshold for CodeCov?

Sorry, I was thinking of this function: https://github.com/Masterminds/semver/blob/2f39fdc11c33c38e8b8b15b1f04334ba84e751f2/version.go#L214

Because of the pre and metadata fields, etc. it's just easier to do all conversions via .String().

Signed-off-by: Todd Short <tshort@redhat.com>

tmshort requested a review from a team as a code owner August 29, 2023 19:34

tmshort commented Aug 29, 2023

View reviewed changes

internal/resolution/variablesources/required_package_test.go Show resolved Hide resolved

tmshort force-pushed the OPRUN-3034 branch from 244596b to ee305fd Compare August 29, 2023 19:36

tmshort changed the title ~~Oprun 3034~~ Commits on Aug 29, 2023 Introduce Masterminds/semver Aug 29, 2023

ncdc changed the title ~~Commits on Aug 29, 2023 Introduce Masterminds/semver~~ Introduce Masterminds/semver Aug 29, 2023

ncdc reviewed Aug 29, 2023

View reviewed changes

api/v1alpha1/operator_types.go Show resolved Hide resolved

internal/resolution/variablesources/required_package_test.go Show resolved Hide resolved

tmshort marked this pull request as draft August 29, 2023 20:37

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Aug 29, 2023

joelanford reviewed Aug 29, 2023

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 29, 2023

tmshort force-pushed the OPRUN-3034 branch 2 times, most recently from 8ca0525 to f63044a Compare August 30, 2023 21:29

tmshort marked this pull request as ready for review August 31, 2023 13:24

grokspawn reviewed Aug 31, 2023

View reviewed changes

tmshort force-pushed the OPRUN-3034 branch from f63044a to 83952dd Compare August 31, 2023 16:06

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 31, 2023

tmshort mentioned this pull request Aug 31, 2023

Update Version regex to support ranges #359

Closed

4 tasks

joelanford mentioned this pull request Aug 31, 2023

[Epic] Support version ranges #379

Closed

Update Version regex to support ranges

e3bb5d5

Fixes operator-framework#345 Add positive and negative test cases. Signed-off-by: Todd Short <tshort@redhat.com>

tmshort force-pushed the OPRUN-3034 branch from 83952dd to d424906 Compare August 31, 2023 17:38

Introduce Masterminds/semver

ac07fe1

Fixes operator-framework#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 <tshort@redhat.com>

tmshort force-pushed the OPRUN-3034 branch from d424906 to ac07fe1 Compare August 31, 2023 18:04

joelanford reviewed Aug 31, 2023

View reviewed changes

fixup! Introduce Masterminds/semver

f005767

Signed-off-by: Todd Short <tshort@redhat.com>

joelanford approved these changes Aug 31, 2023

View reviewed changes

tmshort added this pull request to the merge queue Aug 31, 2023

Merged via the queue into operator-framework:main with commit 628ae95 Aug 31, 2023
10 of 11 checks passed

tmshort deleted the OPRUN-3034 branch September 1, 2023 13:49

tmshort mentioned this pull request Sep 5, 2023

Version Ranges: Introduce Masterminds/semver #346

Closed

m1kola mentioned this pull request Sep 26, 2023

Remove dependency on github.com/blang/semver #430

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Masterminds/semver #374

Introduce Masterminds/semver #374

tmshort commented Aug 29, 2023 •

edited

Loading

codecov bot commented Aug 29, 2023 •

edited

Loading

tmshort commented Aug 29, 2023

ncdc commented Aug 29, 2023

joelanford Aug 29, 2023 •

edited

Loading

tmshort Aug 30, 2023

joelanford Aug 30, 2023

tmshort Aug 30, 2023 •

edited

Loading

tmshort commented Aug 31, 2023

grokspawn Aug 31, 2023

tmshort Aug 31, 2023 •

edited

Loading

tmshort commented Aug 31, 2023

tmshort commented Aug 31, 2023

joelanford Aug 31, 2023

joelanford Aug 31, 2023

tmshort Aug 31, 2023 •

edited

Loading

tmshort Aug 31, 2023

joelanford Aug 31, 2023

joelanford Aug 31, 2023

tmshort Aug 31, 2023 •

edited

Loading

joelanford Aug 31, 2023

tmshort Sep 1, 2023

tmshort Sep 1, 2023

Introduce Masterminds/semver #374

Introduce Masterminds/semver #374

Conversation

tmshort commented Aug 29, 2023 • edited Loading

Description

Reviewer Checklist

codecov bot commented Aug 29, 2023 • edited Loading

Codecov Report

tmshort commented Aug 29, 2023

ncdc commented Aug 29, 2023

joelanford Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmshort Aug 30, 2023 • edited Loading

Choose a reason for hiding this comment

tmshort commented Aug 31, 2023

Choose a reason for hiding this comment

tmshort Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

tmshort commented Aug 31, 2023

tmshort commented Aug 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmshort Aug 31, 2023 • edited Loading

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

tmshort Aug 31, 2023 • edited Loading

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

tmshort commented Aug 29, 2023 •

edited

Loading

codecov bot commented Aug 29, 2023 •

edited

Loading

joelanford Aug 29, 2023 •

edited

Loading

tmshort Aug 30, 2023 •

edited

Loading

tmshort Aug 31, 2023 •

edited

Loading

tmshort Aug 31, 2023 •

edited

Loading

tmshort Aug 31, 2023 •

edited

Loading