Check only for new bundles on add #798
Conversation
Signed-off-by: Ankita Thomas <ankithom@redhat.com>
ankitathomas
force-pushed
the
prune-message
branch
from
a23727d to
d3be374
Compare
Codecov Report
@@ Coverage Diff @@
## master #798 +/- ##
==========================================
- Coverage 51.18% 51.08% -0.11%
==========================================
Files 103 103
Lines 9078 9041 -37
==========================================
- Hits 4647 4619 -28
+ Misses 3551 3545 -6
+ Partials 880 877 -3
Continue to review full report at Codecov.
|
ankitathomas
changed the title
ankitathomas
force-pushed
the
prune-message
branch
from
926a89c to
d16e083
Compare
Signed-off-by: Ankita Thomas <ankithom@redhat.com>
ankitathomas
force-pushed
the
prune-message
branch
from
d16e083 to
12d4f7c
Compare
|
@ankitathomas @kevinrizza @joelanford I think this is a good example of where i'd have expected some sort of "opm add" CI test gating to have prevented the original change(the one that regressed the behavior by erroring out on any pruned bundle) from going in. |
dinhxuanvu
left a comment
dinhxuanvu
left a comment
There was a problem hiding this comment.
Looking good. Just a quick question.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ankitathomas, dinhxuanvu 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 |
openshift-ci
bot
added
the
approved
| version: "1.2.0", | ||
| }, | ||
| }, | ||
| action: actionAdd, |
There was a problem hiding this comment.
Probably outside the scope of this pull request, but it would be nice if we had a way of comparing the contents of the database based on these actions rather than just checking for error conditions.
pkg/lib/registry/registry.go
Outdated
| var err error | ||
| checkForBundleInChannel: | ||
| for _, channel := range bundle.Channels { | ||
| if graph == nil { |
There was a problem hiding this comment.
Is there a reason not to just do this once before we iterate?
| }, | ||
| { | ||
| description: "silentPruneForExistingBundle", | ||
| steps: []step{ |
There was a problem hiding this comment.
Should we also test for the case where we're inserting multiple new bundles into the same graph? Both the failure case (one of those bundles doesn't replace another) and the success case (one of the bundles resets the graph, another gets added on top of it)?
pkg/lib/registry/registry_test.go
Outdated
| wantErr: fmt.Errorf("add prunes bundle unorderedReplaces-1.0.0 (unorderedReplaces-1.0.0) from package testpkg, channel stable: this may be due to incorrect channel head (unorderedReplaces-1.1.0, skips/replaces [])"), | ||
| }, | ||
| { | ||
| description: "silentPruneForExistingBundle", |
There was a problem hiding this comment.
nit: casing should match on all test names.
Signed-off-by: Ankita Thomas <ankithom@redhat.com>
| action: actionAdd, | ||
| }, | ||
| { | ||
| bundles: map[string]bundleDir{ |
6 participants
Check only for pruning of newly added bundles, allow existing bundles to be silently dropped in case of no specified paths
Also, update pruning error for clarity
Closes #799