operator-registry icon indicating copy to clipboard operation
operator-registry copied to clipboard

Add `add_mode` column to package table

Open joelanford opened this issue 3 years ago • 3 comments

Description of the change: This PR introduces:

  • an sqlite migration that adds an add_mode column to the package table.
  • a new global sanity check that's part of the add command that causes failures when attempting to add with a mode that is incompatible with a package's previously used add mode.
  • logic to ensure the add_mode column is correctly set when any of the existing supported add modes is used (replaces, semver, semver-skippatch)

Motivation for the change: This change enshrines our existing guidance that add modes cannot be mixed and matched, by comparing the previously-used add mode with the currently requested add mode and emitting an error if they do not match. This functionality is crucial for ensuring that opm can maintain the integrity of the upgrade graph of each package in the catalog.

This is also a necessary pre-requisite for #949 , which will add a new add mode called "fbc" that will be able to idempotently convert FBC catalogs to sqlite. We need to be able to prevent declarative FBC-derived packages from being tampered with via imperative add commands.

Downstream note: This change will likely need to be backported to all versions we expect to support catalogs containing reverse-migrated packages so that we can ask users to upgrade to the opm binary that contains these extra add_mode checks. Users that use versions of opm without these checks will still be at risk for mixing/matching add modes despite our guidance that they should not.

Reviewer Checklist

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

joelanford avatar Jun 22 '22 17:06 joelanford

Codecov Report

Merging #972 (7db3c0b) into master (0899512) will increase coverage by 0.31%. The diff coverage is 62.19%.

:exclamation: Current head 7db3c0b differs from pull request most recent head f05ca62. Consider uploading reports for the commit f05ca62 to get more accurate results

@@            Coverage Diff             @@
##           master     #972      +/-   ##
==========================================
+ Coverage   52.48%   52.80%   +0.31%     
==========================================
  Files         103      104       +1     
  Lines        9240     9293      +53     
==========================================
+ Hits         4850     4907      +57     
+ Misses       3468     3446      -22     
- Partials      922      940      +18     
Impacted Files Coverage Δ
pkg/registry/channelupdateoptions.go 77.77% <0.00%> (+77.77%) :arrow_up:
pkg/registry/empty.go 0.00% <0.00%> (ø)
pkg/registry/graph.go 20.00% <0.00%> (-1.74%) :arrow_down:
pkg/sqlite/graphloader.go 69.44% <50.00%> (-1.15%) :arrow_down:
pkg/sqlite/load.go 46.86% <50.00%> (+0.58%) :arrow_up:
pkg/sqlite/query.go 64.32% <50.00%> (-0.22%) :arrow_down:
pkg/registry/populator.go 66.97% <56.25%> (+3.81%) :arrow_up:
pkg/sqlite/migrations/014_package_add_mode.go 69.69% <69.69%> (ø)
pkg/registry/bundlegraphloader.go 77.33% <100.00%> (+1.27%) :arrow_up:
pkg/registry/types.go 29.21% <100.00%> (+0.80%) :arrow_up:
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 0899512...f05ca62. Read the comment docs.

codecov[bot] avatar Jun 22 '22 18:06 codecov[bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: joelanford, tylerslaton

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[bot] avatar Jun 22 '22 19:06 openshift-ci[bot]

We definitely need some docs on this, or customers are just not going to understand the interactions here, particularly mapping the error messages into remediation actions.

grokspawn avatar Aug 10 '22 16:08 grokspawn

closing PR as stale. Please re-open if it's still important.

perdasilva avatar Feb 19 '24 13:02 perdasilva