Skip to content
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

(feat): Bundle commutativity #285

Conversation

kevinrizza
Copy link
Member

@kevinrizza kevinrizza commented Apr 17, 2020

opm registry add and opm index add, when presented with multiple
bundles, required that those bundles be in version order when inserting
in replaces mode. This requirement existed because bundles are inserted
into the database in order, and if one of these bundles replaces another
the db schema would reject the insert.

This commit updates the replaces mode insert logic to check the existing
package graph and attempt to insert bundles in order.

As a followup to this commit, once we define a method of inserting in
replaces mode based on a graph input, we should be able to reduce this
to a single db transaction by defining the graph as a set of the
existing db + new input bundles. This would also simplify error handling,
which would no longer need any database transactions.

Description of the change:

Motivation for the change:

Reviewer Checklist

  • Implementation matches the proposed design, or proposal is updated to match implementation
  • Sufficient unit test coverage
  • Sufficient end-to-end test coverage
  • Docs updated or added to /docs
  • Commit messages sensible and descriptive

`opm registry add` and `opm index add`, when presented with multiple
bundles, required that those bundles be in version order when inserting
in replaces mode. This requirement existed because bundles are inserted
into the database in order, and if one of these bundles replaces another
the db schema would reject the insert.

This commit updates the replaces mode insert logic to check the existing
package graph and attempt to insert bundles in order.

As a followup to this commit, once we define a method of inserting in
replaces mode based on a graph input, we should be able to reduce this
to a single db transaction by defining the graph as a set of the
existing db + new input bundles. This would also simplify error handling,
which would no longer need any database transactions.
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 17, 2020
Copy link
Member

@ecordell ecordell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if I just missed it - but there should be at least one test that verifies the commutativity property?

err = i.loadManifestsReplaces(bundle, annotationsFile)
if err != nil {
return err
// TODO: This is relatively inefficient. Ideally, we should be able to use a replaces
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is also my main comment - we have a lot of the lower level stuff in place to do this really efficiently now.

But I don't think that needs to block

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, looking at it it was just going to take a lot more time and effort to reimplement some of the sql layer to do that, so I opted for this initial implementation to get it out there and kicked the can down the road somewhat.

as for a test, not explicitly called out, but the root db populate now generates the database with out of order input: https://github.com/operator-framework/operator-registry/pull/285/files#diff-bac4eb64785fb97243acdc492eedd162R70

@exdx exdx mentioned this pull request Apr 18, 2020
5 tasks
@ecordell
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 19, 2020
@openshift-merge-robot openshift-merge-robot merged commit 71ced38 into operator-framework:master Apr 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants