Fixes replace based upgrades #326
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
Codecov Report
@@ Coverage Diff @@
## main #326 +/- ##
===========================================
+ Coverage 62.58% 82.84% +20.26%
===========================================
Files 21 21
Lines 890 892 +2
===========================================
+ Hits 557 739 +182
+ Misses 241 107 -134
+ Partials 92 46 -46
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| replacesValue, _ := json.Marshal(replacesProperty{Replaces: b.Replaces}) | ||
| props["olm.replaces"] = string(replacesValue) | ||
| replacesValue, _ := json.Marshal(replacesProperty{ | ||
| Replaces: fmt.Sprintf("%s-%s%s%s", bundle.Spec.Catalog.Name, b.Replaces, bundle.Spec.Package, ch.Name), |
There was a problem hiding this comment.
So the idea is that: in the entity properties we have formated replaces (vs raw data from catalogd objects) and don't have to format it in internal/resolution/variablesources/installed_package.go: we just filter entity source by id of the currently installed entity.
However I'm not entirely sure whether this is what we want to do. This limits replaces to a specific catalog and channel.
There was a problem hiding this comment.
In OLM V0, which I believe is be the primary usecase for replace/skips/skipRange based upgrades, we supported cross channel updates. I suspect that we'll want to support cross channel upgrades here as well.
There was a problem hiding this comment.
Yeah, and for semver upgrade support we also want:
- The bundle resolved for an operator during an upgrade can come from a different channel or catalog than the currently installed bundle. (i.e. successors can come from any channel and any catalog)
- The bundle resolved for an operator during an upgrade cannot come from a different package than the currently installed bundle.
So I'm looking into other options to solve this.
There was a problem hiding this comment.
I think it may be less restricting to have channel information be a new constraint rather than included into the replaces field keeping cross channel updates in mind, similar to what the upgrade support brief mentions.
There was a problem hiding this comment.
@ankitathomas for now I'm trying to make minimal changes to fix it. I think this stuff will change a lot soon with semver based upgrades: #326 (comment)
| @@ -140,7 +138,7 @@ func (b *BundleEntity) loadReplaces() error { | |||
| b.mu.Lock() | |||
| defer b.mu.Unlock() | |||
| if b.replaces == nil { | |||
| replaces, err := loadFromEntity[Replaces](b.Entity, "olm.replaces", optional) | |||
| replaces, err := loadFromEntity[Replaces](b.Entity, PropertyBundleReplaces, optional) | |||
|
Ok, here what I did:
For now I'm trying to make minimal changes to fix the issue, but I have a feeling that things will change internall once we introduce semver based upgrades. |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>
Description
Fixes replace based upgrades.
Closes #321
Reviewer Checklist