validate fail for dupe skips+replaces channel entries #1750
Conversation
Signed-off-by: grokspawn <jordan@nimblewidget.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: grokspawn The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| if slices.Contains(entry.Skips, entry.Replaces) { | ||
| return nil, fmt.Errorf("invalid package %q, channel %q: entry %q has identical replaces and skips: %q", c.Package, c.Name, entry.Name, entry.Replaces) | ||
| } | ||
| } |
There was a problem hiding this comment.
👍 Make sense for me my only concern is:
Did we check how many cases do we have that fail in this scenario?
we might need to create a script to validate, what we do if we have FBC catalogs with?
But maybe it will need to see outside of this PR
/lgtm
There was a problem hiding this comment.
We do see one instance in the operatorhubio catalog:
./operatorhubio/latest
FATA[0002] invalid package "grafana-operator", channel "v5": entry "grafana-operator.v5.10.0" has identical replaces and skips: "grafana-operator.v5.9.2"
let's
/hold
this until we can talk to some impacted folks and determine if this is a big enough problem to have to solve NOW.
|
This validation check seems to be very narrowly tailored to "can't both skip and replace the same thing in one entry", which is good! However, I think it very slightly misses the point and the broader problem.
|
This is totally fine in any OLMv1 context, but I'd argue that since it comes with migration side-effects for OLMv0 that it's never OK. In general, we should not have these kind of surprises, and I think it's reasonable to enforce the most-restrictive case here (because it's easier to grow-permissive than -restrictive).
That's a specific flavor of this more general issue. But I'd argue that it is also resolved by preventing the more general issue. |
Description of the change:
opm validatefails when a skip edge exists for a channel which matches the entry's replaces edge.Motivation for the change:
Due to OLMv0 graph mechanics, the skips edge will cause OLMv0 to ignore the bundle version when considering upgrades (since v0 discards graph contribution from skipped bundle versions).
Since the purpose of a replaces edge is to enable upgrade mobility across a graph, allowing the bundle version to be ignored (due to the skips entry) is an error, and potentially results in stranding.
For example, take input
olm.channel:{ "schema": "olm.channel", "name": "stable-v1", "package": "test-operator", "entries": [ { "name": "test-operator-v1.0.0" }, { "name": "test-operator-v1.1.0" }, { "name": "test-operator-v1.1.2" }, { "name": "test-operator-v1.1.4", "replaces": "test-operator-v1.0.0", "skips": [ "test-operator-v1.0.0", "test-operator-v1.1.0", "test-operator-v1.1.2" ] }, { "name": "test-operator-v1.2.0" }, { "name": "test-operator-v1.2.1", "replaces": "test-operator-v1.1.4", "skips": [ "test-operator-v1.1.4", "test-operator-v1.2.0" ] }, { "name": "test-operator-v1.3.0", "replaces": "test-operator-v1.2.1", "skips": [ "test-operator-v1.2.1" ] }, { "name": "test-operator-v1.4.0", "replaces": "test-operator-v1.3.0", "skips": [ "test-operator-v1.3.0" ] } ] }Using a new version of opm which can optionally display OLMv0 graph semantics, you can appreciate that the edges with duplicate replaces/skips will be ignored in the graph (skipped objects are limned in red and ignored edges are red dashed arrows).
Reviewer Checklist
/docs