Skip to content

✨ use single semver library #1565

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

azych
Copy link
Contributor

@azych azych commented Jan 10, 2025

There are currently two libraries being used directly in the code to deal with semver: github.com/Masterminds/semver/v3 and github.com/blang/semver/v4.
This PR tries to elimitate one of them (blang/semver), since the second library seems to be covering all use cases where blang was being used.
This has a direct impact on security as well as readability and general development, because we won't be needing to deal with different types that ultimately handle the same thing.

At this stage, this is meant to be a draft looking for opinions and any considerations which would explain why this shouldn't be done. I have not looked into masterminds NewVersion() vs StrictNewVersion() for example and if there would be a need to use the latter.

Rationale for choosing to go with mastermind's lib in this draft was that we only used two methods from blang that mm covered and used a lot more functionality of mm that blang didin't cover (or at least doesn't seem to cover).

Note: blang/semver still remains as an indirect dependency, potentially because of use in declcfg.

Reviewer Checklist

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

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 10, 2025
Copy link

netlify bot commented Jan 10, 2025

Deploy Preview for olmv1 ready!

Name Link
🔨 Latest commit b3bf459
🔍 Latest deploy log https://app.netlify.com/sites/olmv1/deploys/67810315f0ff730008601dc7
😎 Deploy Preview https://deploy-preview-1565--olmv1.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Jan 10, 2025

Codecov Report

Attention: Patch coverage is 66.66667% with 5 lines in your changes missing coverage. Please review.

Project coverage is 66.65%. Comparing base (fff1896) to head (b3bf459).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
catalogd/internal/version/version.go 0.00% 4 Missing ⚠️
internal/resolve/resolver.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1565      +/-   ##
==========================================
- Coverage   66.68%   66.65%   -0.04%     
==========================================
  Files          57       57              
  Lines        4584     4585       +1     
==========================================
- Hits         3057     3056       -1     
- Misses       1302     1304       +2     
  Partials      225      225              
Flag Coverage Δ
e2e 52.72% <72.72%> (ø)
unit 53.58% <60.00%> (-0.04%) ⬇️

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

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

@azych
Copy link
Contributor Author

azych commented Jan 13, 2025

It was brought up that there are changes in how github.com/blang/semver/v4 and github.com/Masterminds/semver/v3 handles version ranges.

Because OLM v0 used github.com/blang/semver/v4, registry+v1 bundles and their authors might still rely on those specific differences in handling. This means that for as long as registry+v1 bundles are supported by OLM v1 (or we can confirm it otherwise somehow), github.com/blang/semver/v4 needs to remain alongside any other semver handling lib to ensure version ranges are handled in a compatible and expected way.
For additional context, see OLM v1 Version Ranges RFC.

I am closing this PR and will be adding comments with context to the code in a separate one.

@joelanford @bentito @thetechnick

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant