-
Notifications
You must be signed in to change notification settings - Fork 50
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
🌱 use operator-registry's FBC types directly; add Resolver
interface
#1033
🌱 use operator-registry's FBC types directly; add Resolver
interface
#1033
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
c5230bd
to
f23a6a1
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1033 +/- ##
==========================================
- Coverage 73.60% 72.10% -1.50%
==========================================
Files 30 31 +1
Lines 1951 1807 -144
==========================================
- Hits 1436 1303 -133
+ Misses 381 377 -4
+ Partials 134 127 -7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
3664e64
to
a03a969
Compare
a03a969
to
9803223
Compare
Resolver
interface
9803223
to
7b0983c
Compare
Upgrade e2e fails... |
I'm hoping #1050 improves the situation on the upgrade-e2e. I'll rebase after that merges. |
7b0983c
to
72f672e
Compare
Rebased. Let's see if upgrade-e2e passes now. EDIT: It did! 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm to me - just a couple of small questions but nothing that should block this (though some tracking tickets if we want to address anything would be good =))
internal/bundleutil/bundle.go
Outdated
return nil, fmt.Errorf("no package property found in bundle %q", b.Name) | ||
} | ||
|
||
// MetadataFor returns a BundleMetadata for the given bundle. If the provided bundle is nil, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still accurate? Would something like the following be more accurate?
// MetadataFor returns a BundleMetadata for the given bundle. If the provided bundle is nil, | |
// MetadataFor returns a BundleMetadata for the given bundle name and version. If the provided bundle version is nil, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed this in two ways:
- Added
name and version
- Dropped the second sentence because I changed the signature to avoid nil-pointer panic possibilities.
internal/bundleutil/bundle.go
Outdated
|
||
// MetadataFor returns a BundleMetadata for the given bundle. If the provided bundle is nil, | ||
// this function panics. It is up to the caller to ensure that the bundle is non-nil. | ||
func MetadataFor(bundleName string, bundleVersion *bsemver.Version) *ocv1alpha1.BundleMetadata { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it make sense to wrap the bundle in an internal struct with this added method? I can see both ways working, I just don't know how "first class" this method needs to be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
catalogmetadata.Bundle
was the wrapper that this PR removed, somewhat purposefully. What I had noticed is that we started losing parity with the FBC data model (we put channel membership and deprecation information in that wrapper). And that seemed like a recipe for more spaghetti-making when FBC evolves new types and semantics.
So I opted for functions over declcfg.Bundle
rather than a wrapper type with methods.
Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
72f672e
to
b59ac9e
Compare
Most recent push:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Looks like this will make the codebase a lot more manageable!
6cd022e
…or-framework#1033) Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
…or-framework#1033) Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
Signed-off-by: Joe Lanford joe.lanford@gmail.com
Description
This is a code refactoring to:
Reviewer Checklist