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

feat(catalog): add grpc sourcetype to CatalogSources #656

Closed
wants to merge 1 commit into from
Closed

feat(catalog): add grpc sourcetype to CatalogSources #656

wants to merge 1 commit into from

Conversation

ecordell
Copy link
Member

@ecordell ecordell commented Jan 9, 2019

The grpc sourcetype takes a new image field which specifies an image which is expected to serve a registry API over grpc on port 50051.

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 9, 2019
@ecordell
Copy link
Member Author

ecordell commented Jan 9, 2019 via email

@ecordell ecordell changed the title [QIP] feat(catalog): add grpc sourcetype to CatalogSources [WIP] feat(catalog): add grpc sourcetype to CatalogSources Jan 10, 2019
@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 10, 2019
@ecordell ecordell force-pushed the grpc-catalog-source branch from da77153 to ead6afc Compare January 10, 2019 20:55
@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 10, 2019
@ecordell ecordell changed the title [WIP] feat(catalog): add grpc sourcetype to CatalogSources feat(catalog): add grpc sourcetype to CatalogSources Jan 10, 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 10, 2019
@ecordell ecordell force-pushed the grpc-catalog-source branch from ead6afc to 5d361c7 Compare January 10, 2019 21:05
@ecordell
Copy link
Member Author

/retest

@ecordell
Copy link
Member Author

ecordell commented Jan 11, 2019 via email

@ecordell ecordell force-pushed the grpc-catalog-source branch from 5d361c7 to e18d26c Compare January 11, 2019 19:14
@ecordell
Copy link
Member Author

/retest

1 similar comment
@ecordell
Copy link
Member Author

/retest

@ecordell ecordell force-pushed the grpc-catalog-source branch from e18d26c to 11f63a1 Compare January 14, 2019 23:06
@ecordell
feat(catalog): add grpc sourcetype to CatalogSources
123d9c8 
grpc sourcetype takes a new `image` field which specifies an image
which is expected to serve a registry API over grpc on port 50051.
@ecordell ecordell force-pushed the grpc-catalog-source branch from 11f63a1 to 123d9c8 Compare January 14, 2019 23:06
@openshift-ci-robot
Copy link
Collaborator

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

Test name Commit Details Rerun command
ci/prow/e2e-aws 123d9c8 link /test e2e-aws
ci/prow/e2e-aws-olm 123d9c8 link /test e2e-aws-olm

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

njhale
njhale reviewed Jan 15, 2019
View reviewed changes
pkg/controller/operators/catalog/operator.go
resolver resolver.Resolver
subQueue workqueue.RateLimitingInterface
catSrcQueue workqueue.RateLimitingInterface
reconciler reconciler.ReconcilerReconciler
Copy link
Member

Choose a reason for hiding this comment

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

🔥 still prefer ReconcilerFactory.

njhale
njhale reviewed Jan 15, 2019
View reviewed changes
pkg/controller/operators/catalog/operator.go
}
sourceKey := resolver.CatalogKey{Name: catsrc.GetName(), Namespace: catsrc.GetNamespace()}
func() {
Copy link
Member

@njhale njhale Jan 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Why does this need to execute in an anonymous function if the outer function returns immediately after this (ie. No real work is being done between the lock and the return)?

Copy link
Member Author

Choose a reason for hiding this comment

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

For future changes, this keeps the unlock close to to the reason we're locking.

Copy link
Member

Choose a reason for hiding this comment

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

I understand that - was just commenting on this specific case - it makes sense to be consistent with the rest of the codebase.

njhale
njhale reviewed Jan 15, 2019
View reviewed changes
pkg/controller/registry/reconciler/reconciler.go
EnsureRegistryServer(catalogSource *v1alpha1.CatalogSource) error
}

type ReconcilerReconciler interface {
Copy link
Member

Choose a reason for hiding this comment

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

see other comment on ReconcilerReconciler name.

@openshift-ci-robot
Copy link
Collaborator

@njhale: dog image

In response to this:

Looks great overall. A few nits, but nothing to block over.
/lgtm
/bark

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.

njhale
njhale approved these changes Jan 15, 2019
View reviewed changes
Copy link
Member

@njhale njhale 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 overall. A few nits, but nothing to block over.
/lgtm
/bark

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 15, 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

@ecordell
Copy link
Member Author

These changes have been pulled into #670

@ecordell ecordell closed this Jan 18, 2019
@ecordell ecordell deleted the grpc-catalog-source branch January 23, 2019 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@njhale njhale njhale approved these changes

@alecmerdler alecmerdler Awaiting requested review from alecmerdler

Assignees

@njhale njhale

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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

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