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.

Sign up for GitHub

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

Jump to bottom

fix(subscriptions): respect startingCSV #676

Merged
merged 1 commit into from
Jan 31, 2019
Merged

fix(subscriptions): respect startingCSV #676

merged 1 commit into from
Jan 31, 2019

Conversation

njhale
Copy link
Member

@njhale njhale commented Jan 19, 2019

Fixes Subscription resolution to respect the StartingCSV field.

Addresses ALM-851.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 19, 2019
@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 19, 2019
ecordell
ecordell requested changes Jan 20, 2019
View reviewed changes
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.

Looks great! Couple of comments on the tests

pkg/controller/registry/resolver/querier_test.go Outdated
@@ -239,7 +239,7 @@ func TestNamespaceSourceQuerier_FindPackage(t *testing.T) {
q := &NamespaceSourceQuerier{
sources: tt.fields.sources,
}
got, key, err := q.FindPackage(tt.args.pkgName, tt.args.channelName, tt.args.initialSource)
got, key, err := q.FindPackage(tt.args.pkgName, tt.args.channelName, "", tt.args.initialSource)
require.Equal(t, tt.out.bundle, got)
Copy link
Member

Choose a reason for hiding this comment

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

Unit tests for new querier behavior?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do.

test/e2e/subscription_e2e_test.go Outdated
require.Equal(t, v1alpha1.InstallPlanPhaseComplete, fetchedInstallPlan.Status.Phase)

// Wait for csvA to be found
fetchedCSV, err := fetchCSV(t, crc, csvA.GetName(), testNamespace, buildCSVConditionChecker(v1alpha1.CSVPhaseSucceeded, v1alpha1.CSVPhaseFailed))
Copy link
Member

Choose a reason for hiding this comment

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

This could be racy, right? What if by the time this happens, we’ve synced again and updated from csvA to csvB?

Maybe we do an manual approval?

Copy link
Member Author

Choose a reason for hiding this comment

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

You're totally right - I just pushed up what I had locally. I think a manual approval might work.

@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 21, 2019
@ecordell
Copy link
Member

/retest

@njhale njhale force-pushed the fix-startingcsv branch 4 times, most recently from 302cdec to f5faef4 Compare January 24, 2019 01:07
@njhale njhale changed the title [WIP] fix(subscriptions): respect startingCSV fix(subscriptions): respect startingCSV Jan 24, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 24, 2019
@njhale
Copy link
Member Author

njhale commented Jan 24, 2019

/retest

3 similar comments
@njhale
Copy link
Member Author

njhale commented Jan 24, 2019

/retest

@njhale
Copy link
Member Author

njhale commented Jan 24, 2019

/retest

@njhale
Copy link
Member Author

njhale commented Jan 24, 2019

/retest

@njhale
Copy link
Member Author

njhale commented Jan 25, 2019

/retest

@njhale njhale force-pushed the fix-startingcsv branch 2 times, most recently from 5815c6d to 73f698c Compare January 29, 2019 19:59
@njhale
Copy link
Member Author

njhale commented Jan 29, 2019

/retest

@njhale njhale force-pushed the fix-startingcsv branch 2 times, most recently from 4660665 to e758bb4 Compare January 30, 2019 02:09
@njhale
Copy link
Member Author

njhale commented Jan 30, 2019

/retest

ecordell
ecordell reviewed Jan 30, 2019
View reviewed changes
pkg/controller/registry/resolver/querier.go
@@ -56,21 +55,35 @@ func (q *NamespaceSourceQuerier) FindProvider(api opregistry.APIKey) (*opregistr
return nil, nil, fmt.Errorf("%s not provided by a package in any CatalogSource", api)
}

func (q *NamespaceSourceQuerier) FindPackage(pkgName, channelName string, initialSource CatalogKey) (*opregistry.Bundle, *CatalogKey, error) {
func (q *NamespaceSourceQuerier) FindPackage(pkgName, channelName, csvName string, initialSource CatalogKey) (*opregistry.Bundle, *CatalogKey, error) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this would be better as two separate functions, e.g. FindBundle and FindLatestBundle, etc.

ecordell
ecordell reviewed Jan 30, 2019
View reviewed changes
pkg/controller/operators/catalog/operator.go Outdated

logger.Debug("checking if subscriptions need update")

subs, err := o.lister.OperatorsV1alpha1().SubscriptionLister().Subscriptions(namespace).List(labels.Everything())
if err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch - probably happened during the rebase.

ecordell
ecordell reviewed Jan 30, 2019
View reviewed changes
pkg/controller/registry/resolver/resolver.go
@@ -112,7 +111,6 @@ func (r *OperatorsV1alpha1Resolver) ResolveSteps(namespace string, sourceQuerier
if subExists && existingSubscription.Status.CurrentCSV != op.Identifier() {
existingSubscription.Status.CurrentCSV = op.Identifier()
updatedSubs = append(updatedSubs, existingSubscription)
continue
Copy link
Member

Choose a reason for hiding this comment

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

Why are these removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't (and doesn't) look like this continue is skipping any logic between here and the next iteration of the loop.

@ecordell
Copy link
Member

ecordell commented Jan 31, 2019 via email

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 31, 2019
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

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

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-merge-robot openshift-merge-robot merged commit 7e2c6d3 into operator-framework:master Jan 31, 2019
ecordell pushed a commit to ecordell/operator-lifecycle-manager that referenced this pull request Mar 8, 2019
@openshift-merge-robot
Merge pull request operator-framework#676 from njhale/fix-startingcsv
866416c 
fix(subscriptions): respect startingCSV
@njhale njhale added the kind/bug Categorizes issue or PR as related to a bug. label Mar 19, 2019
@njhale njhale deleted the fix-startingcsv branch September 30, 2019 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ecordell ecordell ecordell approved these changes

@alecmerdler alecmerdler Awaiting requested review from alecmerdler

Assignees

@ecordell ecordell

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@njhale @ecordell @openshift-ci-robot @openshift-merge-robot