Skip to content

Fixes semver range parsing for required packages - #423

Merged
m1kola merged 1 commit into
operator-framework:mainfrom
m1kola:RequiredPackages_fixup
Sep 20, 2023
Merged

Fixes semver range parsing for required packages#423
m1kola merged 1 commit into
operator-framework:mainfrom
m1kola:RequiredPackages_fixup

Conversation

@m1kola

Copy link
Copy Markdown
Member

Description

Moving fix from #413 into a separate PR.

Reviewer Checklist

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

@openshift-ciopenshift-ciBot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 20, 2023
@codecov

codecovBot commented Sep 20, 2023

Copy link
Copy Markdown

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.19%⚠️

Comparison is base (cdb99fa) 83.97% compared to head (a603662) 83.78%.

Additional details and impacted files
@@ Coverage Diff @@## main #423 +/- ##
==========================================
- Coverage 83.97% 83.78% -0.19% 
==========================================
Files 27 27 Lines 1073 1073 ==========================================
- Hits 901 899 -2 - Misses 119 120 +1 - Partials 53 54 +1 
FlagCoverage Δ
e2e61.60% <ø> (-0.23%)⬇️
unit80.34% <100.00%> (ø)

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

Files ChangedCoverage Δ
internal/catalogmetadata/types.go100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

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

@m1kola
m1kolaforce-pushed the RequiredPackages_fixup branch from 27273d2 to 9775d31CompareSeptember 20, 2023 11:43

type PackageRequired struct {
property.PackageRequired
SemverRange *bsemver.Range `json:"-"`

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need a pointer to a function here.

err,
)
}
requiredPackage.SemverRange = &semverRange

@m1kolam1kolaSep 20, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the issue is that we were setting SemverRange on a copy and not on the instance from requiredPackages.

I think we have the same issue in master here:

for_, requiredPackage:=rangerequiredPackages {
semverRange, err:=bsemver.ParseRange(requiredPackage.VersionRange)
iferr!=nil {
returnfmt.Errorf("error determining bundle required package semver range for entity '%s': '%w'", b.ID, err)
}
requiredPackage.SemverRange=&semverRange
}

But I believe it is unused currently in master since we do parsing here anyway:

semverRange, err:=bsemver.ParseRange(requiredPackage.VersionRange)
iferr!=nil {
returnnil, err
}

Not going to submit a PR for bundle_entity.go since it is going away as soon as #413 merges.

Comment threadinternal/catalogmetadata/types_test.go Outdated
Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>
@m1kola
m1kolaforce-pushed the RequiredPackages_fixup branch from 9775d31 to a603662CompareSeptember 20, 2023 11:56
@m1kola
m1kola marked this pull request as ready for review September 20, 2023 12:24
@m1kola
m1kola requested a review from a team as a code ownerSeptember 20, 2023 12:24
@openshift-ciopenshift-ciBot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 20, 2023

@perdasilvaperdasilva left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@m1kola
m1kola added this pull request to the merge queue Sep 20, 2023
Merged via the queue into operator-framework:main with commit 1e12a2aSep 20, 2023
@m1kola
m1kola deleted the RequiredPackages_fixup branch September 20, 2023 13:21
LalatenduMohanty pushed a commit to LalatenduMohanty/operator-controller that referenced this pull request Dec 19, 2024
…tor-framework#423)
* update GoDoc comments to be accurate with latest api changes
Signed-off-by: everettraven <everettraven@gmail.com>
* formatting
Signed-off-by: everettraven <everettraven@gmail.com>
* make verify
Signed-off-by: everettraven <everettraven@gmail.com>
* minor grammar fix
Co-authored-by: Jordan Keister <jordan@nimblewidget.com>
* regen crds
Signed-off-by: everettraven <everettraven@gmail.com>
---------
Signed-off-by: everettraven <everettraven@gmail.com>
Co-authored-by: Jordan Keister <jordan@nimblewidget.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@m1kola@perdasilva