Check before insert #384

shawn-hurley · 2020-07-13T20:30:19Z

Description of the change:

adding sanity check before adding a bundle to check it the package/csv/version is already provided or if the exact bundle is already added.
exit code's now can be used to retrieve these specific error scenarios.
This is targeted at index add, but anything that uses the directory loader will get this 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

* Added case where bundle image is different but provides same info * Added case where bundle was exactly the same

gallettilance

minor nits. What do you think of adding a unit test for this?

pkg/lib/indexer/indexer.go

pkg/lib/registry/registry.go

pkg/registry/populator.go

cmd/opm/main.go

shawn-hurley · 2020-07-15T21:01:04Z

/retest

* adding testcases * updated based on feedback

pkg/registry/types.go

pkg/registry/populator.go

cmd/opm/main.go

pkg/lib/registry/registry.go

ecordell · 2020-07-16T16:59:37Z

pkg/registry/populator_test.go

@@ -426,6 +505,18 @@ func TestImageLoading(t *testing.T) {
 	}
 }

+func checkAggErr(aggErr, wantErr error) bool {
+	if a, ok := aggErr.(utilerrors.Aggregate); ok {
+		for _, e := range a.Errors() {


if you pass t here, you can just use require.EqualElements()

I don't think that is true, I am looking through each error to see if one of them is the error that I want, not that all the errors are the one that I want.

ah, I misread. require.Contains would work, but it's not a big deal.

hum looks that this actually compares error strings and not error types:

TestImageLoading/AddExactBundleAlreadyExists: populator_test.go:509: Error Trace: populator_test.go:509 populator_test.go:489 Error: "[Bundle quay.io/prometheus/operator:0.14.0 already exists Bundle already added that provides package and csv]" does not contain "" Test: TestImageLoading/AddExactBundleAlreadyExists

Just an FYI

pkg/registry/populator.go

ecordell · 2020-07-16T17:48:27Z

/lgtm

estroz · 2020-07-16T17:51:34Z

pkg/registry/populator.go

+	images := make(map[string]bool)
+	for _, image := range imagesToAdd {
+		images[image.bundle.BundleImage] = true
+	}


Nit:

Suggested change

images := make(map[string]bool)

for _, image := range imagesToAdd {

images[image.bundle.BundleImage] = true

}

images := make(map[string]struct{})

for _, image := range imagesToAdd {

images[image.bundle.BundleImage] = struct{}{}

}

pkg/registry/types.go

@@ -14,6 +14,24 @@ var (
 	ErrPackageNotInDatabase = errors.New("Package not in database")
 )

+// BundleImageAlreadyAddedErr is an error that describes a bundle is already added


* Cleaning up imports * Updating names to be more clear * code optimizations.

kevinrizza · 2020-07-17T21:19:35Z

/lgtm
/approve

openshift-ci-robot · 2020-07-17T21:19:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kevinrizza, shawn-hurley

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:

~~OWNERS~~ [kevinrizza]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2020-07-17T21:41:18Z

/retest