Semver index insert #247
Semver index insert #247
Conversation
openshift-ci-robot
added
do-not-merge/work-in-progress
openshift-ci-robot
added
the
approved
pkg/lib/registry/registry.go
Outdated
| } | ||
| defer db.Close() | ||
|
|
||
| // TODO: decouple loader from migrator to call migrate separately |
There was a problem hiding this comment.
fyi, migrator is already decoupled and loader.Migrate is just a convenience
pkg/registry/bundlegraphloader.go
Outdated
| // Iterate over existing nodes and compare the new node's version to find the | ||
| // lowest version above it and highest version below it (to insert between these nodes) | ||
| for node := range channelGraph.Nodes { | ||
| fmt.Println(node) |
pkg/sqlite/graphloader.go
Outdated
| } | ||
| } | ||
|
|
||
| func IsPackageNotInDatabaseError(err error) bool { |
There was a problem hiding this comment.
since we're on 1.13 now: https://golang.org/pkg/errors/#Is
| } else { | ||
| nextNode = replace | ||
| } | ||
| } |
There was a problem hiding this comment.
we should catch when the graph isn't what we expect here - where we don't find any "real" replace in the graph
There was a problem hiding this comment.
Is there a difference between "no real" replaces and "no real replaces but has some synthetic replaces"? What about the case where there is no replacement but the first node in the graph has skips defined? Are we saying that's not valid?
There was a problem hiding this comment.
It looks to me like there is a difference -
We expect channel.Nodes[replaces] to exist, but not channel.Nodes[skips]
This block will find the replaces node if it exists, and add all the skip nodes if they exist - which is good. But if the replaces node doesn't exist, this will set replaces to whatever happens to be the last skips node, which would be incorrect (especially if elsewhere we assume replaces exists in channel.Nodes)
There was a problem hiding this comment.
I don't think it will. If the replacement node isn't in the graph, it will just never set nextNode at all and assume the graph ended (since just before this I initialized nextNode as an empty node). In semver insert mode, this implies that either we really are at the last node OR we aren't at the last node and the real replacement wasn't defined properly in the graph. In that case, we can check that the actual replacement chain is as long as the set of real nodes defined in the graph. If they aren't, it means we got to the end early.
But, when I was thinking about this I realized that this code is non-deterministic for graphs built with skippatch mode -- in that mode there are multiple real candidates for nextNode and the graph isn't being inserted properly. Right now this is definitely broken. So I also need to update the nextNode to a list of nodes to iterate over and insert
openshift-ci-robot
removed
the
needs-rebase
pkg/registry/empty.go
Outdated
| @@ -80,6 +80,10 @@ func (EmptyQuery) GetBundlePathsForPackage(ctx context.Context, pkgName string) | |||
| return nil, errors.New("empty querier: cannot get images") | |||
| } | |||
|
|
|||
| func (EmptyQuery) GetBundlesForPackage(ctx context.Context, pkgName string) (map[BundleKey]struct{}, error) { | |||
| return nil, errors.New("empty querier: cannot get images") | |||
There was a problem hiding this comment.
| return nil, errors.New("empty querier: cannot get images") | |
| return nil, errors.New("empty querier: cannot get bundles") |
| } else { | ||
| nextNode = replace | ||
| } | ||
| } |
There was a problem hiding this comment.
It looks to me like there is a difference -
We expect channel.Nodes[replaces] to exist, but not channel.Nodes[skips]
This block will find the replaces node if it exists, and add all the skip nodes if they exist - which is good. But if the replaces node doesn't exist, this will set replaces to whatever happens to be the last skips node, which would be incorrect (especially if elsewhere we assume replaces exists in channel.Nodes)
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
kevinrizza
force-pushed
the
semver-mode
branch
2 times, most recently
from
152f113
to
3f2589b
Compare
| expectedGraph *Package | ||
| skipPatch bool | ||
| }{ | ||
| { // Add bundle to head of channels when replaces is defined |
There was a problem hiding this comment.
The name of the test case is confusing since it's not using replaces, I'm not sure what the distinction is between this and the next case in semver mode.
There was a problem hiding this comment.
Yeah it's a fair point. My intention was to ensure (via a codified test) that the bundle graph loader wasn't using other input (like the bundle image path) to derive the replaces field defined for existing operators. But I'm fine with just deleting this test given that the input is effectively the same with that assumption.
| }, | ||
| newDefaultChannel: "", | ||
| }, | ||
| { // Add bundle to head of channel when replaces defines a bundle that doesn't exist |
There was a problem hiding this comment.
Again, this is a confusing description. The bundle doesn't have replaces field, only "version"
| }, | ||
| newDefaultChannel: "", | ||
| }, | ||
| { // Add a package already in the graph, expect an error |
There was a problem hiding this comment.
nit: add "bundle" already in the graph
| Name: "etcd", | ||
| DefaultChannel: "alpha", | ||
| Channels: map[string]Channel{ | ||
| "alpha": {Head: BundleKey{CsvName: "etcdoperator.v0.9.3", Version: "0.9.3"}, |
There was a problem hiding this comment.
what's the right graph for skippatch mode? there shouldn't be 0.9.0 or 0.9.2 entry, right?
Should alpha just be:
0.9.3 -> (0.9.0, 0.9.1, 0.9.2, 0.6.1)
0.9.3 skips 0.9.2, so it should skip 0.9.1 (skipped by 0.9.2) transitively
0.9.3 skips 0.9.0, so it should skip 0.6.1 (replaced by 0.9.0) transitively (I think this is arguable)
either way I don't think there should be an entry for 0.9.2, since it's definitively skipped in skippatch mode.
Does this definition of skippatch mode make sense: "If I add 0.Y.Z in skip patch mode, anything that would have updated to any 0.Y.W should instead update directly to 0.Y.Z"?
There was a problem hiding this comment.
Spoke a little about this with Evan, my opinion is that non-latest z versions should not be considered "real" nodes in the graph in skip-patch mode.
Updated the pr to reflect that in the bundle updater.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ecordell, kevinrizza 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 |
| return SemVerMode | ||
| case "semver-skippatch": | ||
| return SkipPatchMode | ||
| default: |
There was a problem hiding this comment.
If mode is coming from user input, I'd be worried about typos causing frustrating behavior. It makes sense to default the mode flag to "replaces" if omitted, but GetModeFromString("semver-skipatch") should not return ReplacesMode.
This adds a new flag to `opm index add` and `opm registry add` which defines two new insert modes to add bundles to the existing update graph Rather than rely on parsing the CSV for the replaces field to define the channel update graph explicitly, in these new modes `opm` implicitly infers the update graph based on the version attached to the bundle and semantic versioning rules (https://semver.org/#summary)
|
/lgtm |
(feat) Semver insert modes
This adds a new flag to
opm index addandopm registry addwhichdefines two new insert modes to add bundles to the existing update graph
Rather than rely on parsing the CSV for the replaces field to define the
channel update graph explicitly, in these new modes
opmimplicitlyinfers the update graph based on the version attached to the bundle and
semantic versioning rules (https://semver.org/#summary)