⚠ use bundle name and version instead of image in (cluster)extension status #679
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #679 +/- ##
==========================================
- Coverage 64.01% 63.50% -0.51%
==========================================
Files 22 22
Lines 1370 1403 +33
==========================================
+ Hits 877 891 +14
- Misses 442 459 +17
- Partials 51 53 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Looks like an issue with the Go version used by our Tilt setup: Hopefully fixed here: operator-framework/tilt-support#6 |
There was a problem hiding this comment.
Thanks for the PR, Joe! Just a small nit.
/lgtm
/approve
Two more follow ups, which we need not address right away and is for future:
- Both Installed and Resolved Bundle can be an array - this may make more sense when we support direct installation from multiple sources. Thinking it from SRE needs, where they would like to augment the operator bundle, with say a telemetry/monitoring stack which is a part of separate bundle but the same Extension entity.
- Can the BundleMetadata also include a type - I know we discussed that its not necessary, but thinking about it again it would make sense if we support direct install with multiple sources, where its not necessary that all the contents are from the same version of the bundle. Even if the bundle is same, source could be different - given there is no resolution, bundle uniqueness need not necessarily be followed.
This is again assuming that we extract the version from the bundle, and will continue populating similar to how we do with resolution.
Supporting multiple installations is not a concern for now, and can be handled when we implement that feature.
| @@ -505,3 +503,14 @@ func (r *ExtensionReconciler) resolve(ctx context.Context, extension ocv1alpha1. | |||
| }) | |||
| return resultSet[0], nil | |||
| } | |||
|
|
|||
| func bundleMetadataFor(bundle *catalogmetadata.Bundle) *ocv1alpha1.BundleMetadata { | |||
| ver, err := bundle.Version() | |||
There was a problem hiding this comment.
Nit: though bundle cannot be nil after resolution (we must already be having a check for it before), can a check also be added here to avoid nil pointer errors.
There was a problem hiding this comment.
I was thinking about this as well. If we pass a nil pointer into this function, what is our expectation?
- We could panic on the basis that we always expect to pass a non-nil pointer and a panic is an indication of a programmer error. I could see making that more explicit by checking for nil and panicking with a custom message.
- We could return an error from this function, but then we have to consider what we'd want the caller to do with it. It doesn't seem all that great to just return that error up the stack.
- We could return a
nilBundleMetadata, which seems fairly reasonable. At that point, we could call it essentially everywhere that we setInstalledBundleorResolvedBundle.
I'm between 1 and 3. WDYT?
There was a problem hiding this comment.
I'm leaning towards 3 personally. I think it makes logical sense that if a nil value is provided as an input to this function it returns a nil response because if there is no bundle there can be no bundle metadata.
In addition to whichever approach is taken, could we also add a comment to document the expected behavior of this function?
There was a problem hiding this comment.
My concern (re: option 3) would be side-effects if nil is not handled properly in all callers of this function. If we document that this produces nil for nil input, then it behooves the caller to handle properly. If undocumented, I think this is this function's responsibility to impose sanity (so option 1).
There was a problem hiding this comment.
Leaning towards option (1) too, given this is going to be internal, and could only be a programmer's error when they expect a bundle to be not nil, and it ends up being so - as in the previous instances we are explicitly setting the installedResource to be nil.
To maintain consistency, we could follow (1) do a check and panic if its nil, or follow (3) not explicitly set nil everywhere else, instead call this function.
There was a problem hiding this comment.
I'll go with option (1):
- Add an explicit panic, so that it is clear to our future selves what the intent is
- Add a comment on the function signature that describes the expectations/behavior.
| InstalledBundle *BundleMetadata `json:"installedBundle,omitempty"` | ||
| // +optional | ||
| ResolvedBundleResource string `json:"resolvedBundleResource,omitempty"` | ||
| ResolvedBundle *BundleMetadata `json:"resolvedBundle,omitempty"` |
There was a problem hiding this comment.
Can we make this change on the ClusterExtension API without it being considered a breaking change to the resultant CRD?
I can see an argument for not being concerned about this, but I know in the past we've talked about trying to adhere to CRD versioning standards even though we are in a v0/alpha state with this project where by semver breaking changes are allowed to happen. Is adhering to the idea of not making breaking changes to a CRD still something we want to focus on moving forward?
As an aside, I don't see this as a problem for the Extension API as I don't think we have cut a release with that API available yet.
There was a problem hiding this comment.
We could if we had to. We could add this field and deprecate the old one. I don't think it's worth it to go to the trouble in an alpha API that we'll likely drop in the next couple of months.
I suppose you could argue, "why make the change at all if we're planning to remove the API?". I'd say that:
ClusterExtensionis still more mature, so making this change there is essentially a preview for existing users ofClusterExtension- It's easier, at least in this case, to keep
ClusterExtensionandExtensionfunctionality aligned in the areas where there is no fundamental difference between the two.
There was a problem hiding this comment.
I don't think it's worth it to go to the trouble in an alpha API that we'll likely drop in the next couple of months
Totally fair. I just wanted to explicitly call this out to make sure that we were making this breaking change as a conscious decision since we have intentionally tried to avoid those in the past, even on our alpha APIs.
I do think in general, we should err on the side of not making breaking changes due to potential side effect to the cluster/clients but I do understand that it should be okay since we are in an alpha state both as a project and for the APIs.
joelanford
force-pushed
the
status-bundle-refs
branch
from
95fa431 to
252cb87
Compare
|
New changes are detected. LGTM label has been removed. |
joelanford
added
the
do-not-merge/hold
|
Holding until #683 merges. |
…atus Signed-off-by: Joe Lanford <[email protected]>
joelanford
force-pushed
the
status-bundle-refs
branch
from
252cb87 to
e5f35f2
Compare
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
joelanford
removed
the
do-not-merge/hold
|
I chatted with @ankitathomas and she suggested moving ahead with this because she is tied up on other issues and can rebase #683 after this merges. |
9fd9b0b
Description
Rename
.status.{installed|resolved}BundleResourceto.status.{installed|resolved}Bundleand convert from astringto a struct with{Name string, Version string}Fixes #270
There are other follow-on improvements we need to think about, that are mentioned in the last few comments of #270:
.status.installedBundlevalue. But we might update the status with resolution failure issues if that bundle removal causes the extension to longer match anything in the new catalog.Appspec remains identical, but the name or version changes, that would look like a different bundle to the extension controller and we would end up updating the status with that newly resolved bundle's metadata.Reviewer Checklist