feat(resolver): filtering deprecated bundles in resolver #1673

gallettilance · 2020-07-22T20:29:32Z

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

gallettilance · 2020-07-22T20:30:07Z

waiting on operator-framework/operator-registry#397

pkg/controller/registry/resolver/operators.go

ecordell · 2020-07-30T14:19:21Z

pkg/controller/registry/resolver/resolver.go

@@ -73,6 +73,8 @@ func (r *SatResolver) SolveOperators(namespaces []string, csvs []*v1alpha1.Clust
 			predicates = append(predicates, WithChannel(sub.Spec.Channel))
 		}

+		predicates = append(predicates, WithoutDeprecatedProperty())


this applies for finding updates / new installs, but we also need this predicate when searching for dependency candidates - see line 302 in getBundleInstallables

dinhxuanvu · 2020-07-30T15:39:44Z

/hold

dinhxuanvu · 2020-07-30T16:12:30Z

/retest

dinhxuanvu · 2020-07-30T16:13:35Z

Looking this is missing some go mod vendor action because I don't see any new changes from registry being pulled into this PR.

dinhxuanvu · 2020-07-30T18:34:24Z

This PR can be merged when it is lgtm and approve. I remove the hold.
/hold cancel

anik120

Have a couple of comments @gallettilance

anik120 · 2020-07-30T19:23:35Z

pkg/controller/registry/resolver/cache.go

@@ -416,6 +416,18 @@ func WithPackage(pkg string) OperatorPredicate {
 	}
 }

+func WithoutDeprecatedProperty() OperatorPredicate {
+	return func(o *Operator) bool {
+		props := o.bundle.GetProperties()


nit: properties, since you've already used properties here

there is only one deprecated property per bundle

Isn't GetProperties returning properties (irrespective of whether those properties are deprecated or not)? And there's a for loop after this that's iterating on the properties we're getting from GetProperties

return func(o *Operator) bool { properties := o.bundle.GetProperties() for _, property := range properties { if property is deprecated { return false } } return true }

reads much better

Alternatively, you could eliminate the variable properties completely:

for _, p := range o.bundle.GetProperties()

@anik120 I thought you were talking about the function name - this makes sense to me

pkg/controller/registry/resolver/util_test.go

anik120 · 2020-07-30T19:26:26Z

pkg/controller/registry/resolver/operators.go

@@ -297,8 +297,6 @@ func NewOperatorFromBundle(bundle *api.Bundle, startingCSV string, sourceKey reg
 		return op, nil
 	}

-


Looks like this file hasn't really been changed and just has noises.

different go formatting I guess

Would reduce noise in this PR if this was reverted back.

gallettilance · 2020-07-30T20:01:30Z

/retest

benluddy · 2020-07-30T20:50:25Z

/retest

ecordell

/lgtm

openshift-ci-robot · 2020-07-31T14:29:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ecordell, gallettilance

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~~ [ecordell]

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-31T14:35:47Z

/retest

Please review the full test history for this PR and help us cut down flakes.

anik120 · 2020-07-31T15:02:05Z

/lgtm

gallettilance · 2020-07-31T15:10:14Z

/retest

gallettilance · 2020-07-31T15:29:19Z

/retest

openshift-bot · 2020-07-31T15:38:19Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-31T15:40:52Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-31T15:53:56Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-31T16:20:02Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-31T16:32:56Z

/retest

Please review the full test history for this PR and help us cut down flakes.

dinhxuanvu · 2020-07-31T18:44:54Z

/retest

gallettilance · 2020-07-31T19:07:47Z

/retest

gallettilance · 2020-07-31T19:20:38Z

/retest

benluddy · 2020-07-31T19:21:03Z

pkg/controller/registry/resolver/cache_test.go:310:35: not enough arguments in call to apiSetToProperties
	have (map["github.com/operator-framework/operator-registry/pkg/registry".APIKey]struct {}, nil)
	want (APISet, APISet, bool)

https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/operator-framework_operator-lifecycle-manager/1673/pull-ci-operator-framework-operator-lifecycle-manager-master-unit/1289276602584141824#1:build-log.txt%3A39

If these are passing locally, you probably need to rebase and check this out.

dinhxuanvu · 2020-07-31T20:53:05Z

/retest

openshift-ci-robot · 2020-07-31T23:00:21Z

@gallettilance: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/unit	`8cd1b77`	link	`/test unit`
ci/prow/e2e-aws-console-olm	`8cd1b77`	link	`/test e2e-aws-console-olm`
ci/prow/e2e-aws-olm	`8cd1b77`	link	`/test e2e-aws-olm`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

dinhxuanvu · 2020-07-31T23:10:47Z

Replaced by #1699

openshift-ci-robot added the do-not-merge/work-in-progress label Jul 22, 2020

openshift-ci-robot requested review from Bowenislandsong and kevinrizza July 22, 2020 20:29

dinhxuanvu added the area/dependency label Jul 23, 2020

gallettilance mentioned this pull request Jul 23, 2020

Adding ability to deprecate bundles operator-framework/operator-registry#397

Merged

5 tasks

ecordell force-pushed the new-resolver branch 3 times, most recently from 807be79 to e74f576 Compare July 26, 2020 10:51

openshift-ci-robot added the needs-rebase label Jul 26, 2020

ecordell force-pushed the new-resolver branch 3 times, most recently from 64fc053 to 2467b25 Compare July 28, 2020 14:12

gallettilance force-pushed the deprecated-filter branch 2 times, most recently from 33bdf15 to 6fa52e8 Compare July 30, 2020 14:00

openshift-ci-robot removed the needs-rebase label Jul 30, 2020

ecordell reviewed Jul 30, 2020

View reviewed changes

gallettilance force-pushed the deprecated-filter branch from 6fa52e8 to fb80c64 Compare July 30, 2020 14:45

dinhxuanvu changed the title ~~WIP Updating: filtering deprecated bundles in resolver~~ feat(resolver): filtering deprecated bundles in resolver Jul 30, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress label Jul 30, 2020

openshift-ci-robot added the do-not-merge/hold label Jul 30, 2020

dinhxuanvu changed the base branch from new-resolver to master July 30, 2020 15:43

openshift-ci-robot removed the do-not-merge/hold label Jul 30, 2020

gallettilance force-pushed the deprecated-filter branch from fb80c64 to 4988c89 Compare July 30, 2020 19:15

anik120 requested changes Jul 30, 2020

View reviewed changes

gallettilance force-pushed the deprecated-filter branch from 4988c89 to f491252 Compare July 31, 2020 14:20

openshift-ci-robot added the needs-rebase label Jul 31, 2020

feat(resolver): filtering deprecated bundles in resolver

8cd1b77

gallettilance force-pushed the deprecated-filter branch from f491252 to 8cd1b77 Compare July 31, 2020 14:23

openshift-ci-robot removed the needs-rebase label Jul 31, 2020

ecordell approved these changes Jul 31, 2020

View reviewed changes

openshift-ci-robot assigned ecordell Jul 31, 2020

openshift-ci-robot added the lgtm label Jul 31, 2020

openshift-ci-robot added the approved label Jul 31, 2020

openshift-ci-robot assigned anik120 Jul 31, 2020

dinhxuanvu closed this Jul 31, 2020

		@@ -297,8 +297,6 @@ func NewOperatorFromBundle(bundle *api.Bundle, startingCSV string, sourceKey reg
		return op, nil
		}

feat(resolver): filtering deprecated bundles in resolver #1673

feat(resolver): filtering deprecated bundles in resolver #1673

Conversation

gallettilance commented Jul 22, 2020

gallettilance commented Jul 22, 2020

Choose a reason for hiding this comment

dinhxuanvu commented Jul 30, 2020

dinhxuanvu commented Jul 30, 2020

dinhxuanvu commented Jul 30, 2020

dinhxuanvu commented Jul 30, 2020

anik120 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gallettilance commented Jul 30, 2020

benluddy commented Jul 30, 2020

ecordell left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Jul 31, 2020

openshift-bot commented Jul 31, 2020

anik120 commented Jul 31, 2020

gallettilance commented Jul 31, 2020

gallettilance commented Jul 31, 2020

openshift-bot commented Jul 31, 2020

openshift-bot commented Jul 31, 2020

openshift-bot commented Jul 31, 2020

openshift-bot commented Jul 31, 2020

openshift-bot commented Jul 31, 2020

dinhxuanvu commented Jul 31, 2020

gallettilance commented Jul 31, 2020

gallettilance commented Jul 31, 2020

benluddy commented Jul 31, 2020

dinhxuanvu commented Jul 31, 2020

openshift-ci-robot commented Jul 31, 2020

dinhxuanvu commented Jul 31, 2020