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

[deprecation]: Deprecate CatalogSourceConfig API #302

Merged
merged 1 commit into from Apr 19, 2020
Merged

[deprecation]: Deprecate CatalogSourceConfig API #302

merged 1 commit into from Apr 19, 2020

Conversation

anik120
Copy link
Contributor

@anik120 anik120 commented Apr 16, 2020

Description of the change:

This PR removes the CatalogSourceConfig API in it's entirety.

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

@anik120
Copy link
Contributor Author

anik120 commented Apr 16, 2020

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 16, 2020
@anik120 anik120 force-pushed the deprecate-csc branch 3 times, most recently from 7b737bd to 06b17cf Compare April 16, 2020 00:53
@anik120
Copy link
Contributor Author

anik120 commented Apr 16, 2020

/retest

@anik120
Copy link
Contributor Author

anik120 commented Apr 16, 2020

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 16, 2020
@anik120
Copy link
Contributor Author

anik120 commented Apr 16, 2020

/test e2e-aws

kevinrizza
kevinrizza reviewed Apr 16, 2020
View reviewed changes
pkg/operatorsource/configuring.go Outdated
@@ -105,9 +105,9 @@ func (r *configuringReconciler) Reconcile(ctx context.Context, in *v1.OperatorSo
}

// Now that we have updated the datastore, let's check if the opsrc is new.
// If it is, let's force a resync for CatalogSourceConfig.
// If it is, let's force a resync for CatalogSources.
if isResyncNeeded {
Copy link
Member

Choose a reason for hiding this comment

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

This whole section of logic seems unnecessary now, right? I don't think this comment is correct, because the resyncer updates the cached app-registry info that the catalogsourceconfig controller uses to know about what metadata was pulled. Since they don't exist anymore, the whole cache sync just isn't needed anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kevinrizza that's what I thought too, but when I removed this the catalogsource that was being created from the opsrc was missing the packages it is supposed to have. I discovered that after this test failed when this logic was not there: https://github.com/operator-framework/operator-marketplace/blob/master/pkg/operatorsource/configuring_test.go#L27

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kevinrizza turns out that was because of an Expect() in the test, which I have removed, so the resync logic has been removed. FYI we still need to WriteMetaDataToDataStore().
Updated PR.

test/testsuites/watchtests.go
func testRestoreCscService(t *testing.T) {
err := deleteCheckRestoreChild(t, "Service", v2.CatalogSourceConfigKind)
assert.NoError(t, err, cscErrMsg)
}
Copy link
Member

Choose a reason for hiding this comment

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

Not blocking, just had a comment about the cache sync that exists between the two controllers -- I think we can just rip out any reference to the cache. But if that is a non trivial amount of work, then let's at least update the comments to reflect that more accurately

@anik120 anik120 force-pushed the deprecate-csc branch 2 times, most recently from 023030f to 8dfd36e Compare April 16, 2020 20:18
@kevinrizza
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 16, 2020
@anik120
Copy link
Contributor Author

anik120 commented Apr 17, 2020

/retest

ecordell
ecordell reviewed Apr 17, 2020
View reviewed changes
cmd/manager/main.go
}
migrator := migrator.New(clientGo)
err = migrator.Migrate()
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.

this can be a followup pr, but if the migration fails we should probably retry it a couple of times.

(failing to migrate is not the worst, the crd will just stick around)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ecordell till now I haven't seen any error that could be non-deterministic. If we fail once, we're most likely going to fail all the time. I'm not sure if blocking the operator because we're trying multiple times is better than failing fast and reporting the error.

@ecordell
Copy link
Member

/approve

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 17, 2020
@openshift-bot
Copy link
Contributor

/retest

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

7 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@anik120 anik120 force-pushed the deprecate-csc branch 3 times, most recently from bdbcd2d to 44612e7 Compare April 18, 2020 18:42
@anik120
[deprecation]: Deprecate CatalogSourceConfig API
0c2dfde 
This PR removes the CatalogSourceConfig API in it's entirety.
@anik120
Copy link
Contributor Author

anik120 commented Apr 18, 2020

/test e2e-aws

ecordell
ecordell reviewed Apr 19, 2020
View reviewed changes
pkg/migrator/migrator.go
return err
}
labels := catsrc.Labels
delete(labels, builders.CscOwnerNameLabel)
Copy link
Member

Choose a reason for hiding this comment

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

This removes the labels, but I thought there were actual ownerrefs on the catalogsources? Is that not true?

Copy link
Contributor Author

@anik120 anik120 Apr 19, 2020 •
edited

Choose a reason for hiding this comment

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

@ecordell looks like we just set the labels: https://github.com/operator-framework/operator-marketplace/blob/master/pkg/grpccatalog/grpccatalog.go#L116-L119

Copy link
Member

Choose a reason for hiding this comment

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

We don't use ownerrefs because catalogsourceconfigs are created across namespaces

@ecordell
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 19, 2020
@openshift-merge-robot openshift-merge-robot merged commit 88d95b5 into operator-framework:master Apr 19, 2020
anik120 added a commit to anik120/openshift-tests that referenced this pull request Apr 20, 2020
@anik120
[marketplace] Remove CSC related tests
6e430e7 
The CatalogSourceConfig is being deprecated from Openshift 4.5.
This PR removes the CSC related tests.
Ref: operator-framework/operator-marketplace#302
@anik120 anik120 mentioned this pull request Apr 20, 2020
Merged
anik120 added a commit to anik120/openshift-tests that referenced this pull request Apr 20, 2020
@anik120
[marketplace] Remove CSC related tests
6c87afa 
The CatalogSourceConfig is being deprecated from Openshift 4.5.
This PR removes the CSC related tests.
Ref: operator-framework/operator-marketplace#302
anik120 added a commit to anik120/origin that referenced this pull request Apr 20, 2020
@anik120
[marketplace] Remove CSC related tests
fada86c 
The CatalogSourceConfig is being deprecated from Openshift 4.5.
This PR removes the CSC related tests.
Ref: operator-framework/operator-marketplace#302
@anik120 anik120 mentioned this pull request Apr 20, 2020
Merged
anik120 added a commit to anik120/origin that referenced this pull request Apr 20, 2020
@anik120
[marketplace] Remove CSC related tests
d624ec9 
The CatalogSourceConfig is being deprecated from Openshift 4.5.
This PR removes the CSC related tests.
Ref: operator-framework/operator-marketplace#302
anik120 added a commit to anik120/origin that referenced this pull request Apr 20, 2020
@anik120
[marketplace] Remove CSC related tests
cc72fb1 
The CatalogSourceConfig is being deprecated from Openshift 4.5.
This PR removes the CSC related tests.
Ref: operator-framework/operator-marketplace#302
anik120 added a commit to anik120/origin that referenced this pull request Apr 20, 2020
@anik120
[marketplace] Remove CSC related tests
eee6caf 
The CatalogSourceConfig is being deprecated from Openshift 4.5.
This PR removes the CSC related tests.
Ref: operator-framework/operator-marketplace#302
anik120 added a commit to anik120/origin that referenced this pull request Apr 20, 2020
@anik120
[marketplace] Remove CSC related tests
97665d2 
The CatalogSourceConfig is being deprecated from Openshift 4.5.
This PR removes the CSC related tests.
Ref: operator-framework/operator-marketplace#302
anik120 added a commit to anik120/origin that referenced this pull request Apr 20, 2020
@anik120
[marketplace] Remove CSC related tests
d147086 
The CatalogSourceConfig is being deprecated from Openshift 4.5.
This PR removes the CSC related tests.
Ref: operator-framework/operator-marketplace#302
anik120 added a commit to anik120/origin that referenced this pull request Apr 21, 2020
@anik120
[marketplace] Remove CSC related tests
5d4d693 
The CatalogSourceConfig is being deprecated from Openshift 4.5.
This PR removes the CSC related tests.
Ref: operator-framework/operator-marketplace#302
anik120 added a commit to anik120/origin that referenced this pull request Apr 21, 2020
@anik120
Bug 1826071: [marketplace] Remove CSC related tests
dc6839c 
The CatalogSourceConfig is being deprecated from Openshift 4.5.
This PR removes the CSC related tests.
Ref: operator-framework/operator-marketplace#302
tbuskey pushed a commit to tbuskey/openshift-tests that referenced this pull request Apr 23, 2020
@anik120 @tbuskey
[marketplace] Remove CSC related tests
8389c09 
The CatalogSourceConfig is being deprecated from Openshift 4.5.
This PR removes the CSC related tests.
Ref: operator-framework/operator-marketplace#302
@tiraboschi tiraboschi mentioned this pull request May 1, 2020
Merged
kubevirt-bot pushed a commit to kubevirt/hyperconverged-cluster-operator that referenced this pull request May 5, 2020
@tiraboschi
Avoid CatalogSourceConfig (#553)
2293a1a 
CatalogSourceConfig has been removed (not deprecated) on
OLM side with
operator-framework/operator-marketplace#302
Avoid using it and use only OperatorSource in all the
marketplace based deployment scripts.
The OperatorSource will implicitly trigger the creation of
a CatalogSource with the same name.

Signed-off-by: Simone Tiraboschi <stirabos@redhat.com>
kubevirt-bot pushed a commit to kubevirt/hyperconverged-cluster-operator that referenced this pull request May 5, 2020
@orenc1 @tiraboschi
Kustomize fix (#558)
114c5ef 
* Avoid CatalogSourceConfig

CatalogSourceConfig has been removed (not deprecated) on
OLM side with
operator-framework/operator-marketplace#302
Avoid using it and use only OperatorSource in all the
marketplace based deployment scripts.
The OperatorSource will implicitly trigger the creation of
a CatalogSource with the same name.

Signed-off-by: Simone Tiraboschi <stirabos@redhat.com>

* kustomize updates:
1. fix targetNamespace in OperatorGroup to reflect the desired namespace.
2. fix secret creation containing the quay token - quotes removed.
3. added deployment examples in readme file.

Signed-off-by: orenc1 <ocohen@redhat.com>

Co-authored-by: Simone Tiraboschi <stirabos@redhat.com>
anik120 added a commit to anik120/operator-marketplace that referenced this pull request Sep 10, 2020
@anik120
Bug 1877564: Sync remote app-registry
f05f049 
The syncing of remote app-registry was removed by operator-framework#302 since it was
tied to the syncing of the child CatalogSourceConfig which was removed
in operator-framework#302. This PR puts the syncing of remote app-registry back in, minus
the syncing of the non-existing child catalogSourceConfig.
@anik120 anik120 mentioned this pull request Sep 10, 2020
Merged
5 tasks
anik120 added a commit to anik120/operator-marketplace that referenced this pull request Sep 10, 2020
@anik120
Bug 1877564: Sync remote app-registry
0b7c284 
The syncing of remote app-registry was removed by operator-framework#302 since it was
tied to the syncing of the child CatalogSourceConfig which was removed
in operator-framework#302. This PR puts the syncing of remote app-registry back in, minus
the syncing of the non-existing child catalogSourceConfig.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ecordell ecordell ecordell left review comments

@kevinrizza kevinrizza kevinrizza left review comments

@aravindhp aravindhp Awaiting requested review from aravindhp

Assignees

@ecordell ecordell

@kevinrizza kevinrizza

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@anik120 @kevinrizza @ecordell @openshift-ci-robot @openshift-bot @awgreene @openshift-merge-robot