search

google / knative-gcp

GCP event implementations to use with Knative Eventing.
https://github.com/knative/eventing
Apache License 2.0
160 stars 74 forks source link

Make BrokerCell's Targets support things other than Brokers. #2029

Closed Harwayne closed 3 years ago

Harwayne commented 3 years ago

Proposed Changes

Release Note

NONE
knative-prow-robot commented 3 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Harwayne

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](https://github.com/google/knative-gcp/blob/master/OWNERS)~~ [Harwayne] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
Harwayne commented 3 years ago

/retest

Harwayne commented 3 years ago

/retest

yolocs commented 3 years ago

Do we have an issue for this? I'd like to learn more about the motivation.

Harwayne commented 3 years ago

Do we have an issue for this? I'd like to learn more about the motivation.

I didn't make an issue for these. The eventual goal is #2034, Channels are backed by BrokerCells. I was hoping I could finish over the holidays, but didn't make it. It entailed #2026, #2027, and #2028. With #2029, #2030, #2032, and #2034 still outstanding.

Created #2037 to track the overall issue.

Harwayne commented 3 years ago

Friendly ping, now that release-0.20 has been cut.

knative-metrics-robot commented 3 years ago

The following is the coverage report on the affected files. Say /test pull-google-knative-gcp-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/broker/config/cache.go 79.3% 74.3% -5.0
pkg/broker/config/key.go 78.1% 81.5% 3.4
pkg/broker/config/memory/memory.go 98.0% 98.0% 0.0
pkg/broker/handler/fanout.go 89.1% 90.9% 1.8
zhongduo commented 3 years ago

/retest /lgtm /hold My latest comments are optional, please unhold if you don't want to fix it.

zhongduo commented 3 years ago

Make sense.

On Fri, Jan 15, 2021 at 1:52 PM Adam Harwayne notifications@github.com wrote:

@Harwayne commented on this pull request.

In pkg/broker/config/key.go https://github.com/google/knative-gcp/pull/2029#discussion_r558511772:

  • if ctt, err := validateCellTenantTypeFromString(ts); err != nil {
  • return nil, err
  • } else {
  • t = ctt
  • }

I thought about it, but have a strong dislike for when := and = would cause issues. In this case, the compiler would still catch the = error, as t would be unused, but even so, I prefer it this more explicit way. What do you think?

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/google/knative-gcp/pull/2029#discussion_r558511772, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACE6CNAAMPHAJO6DUP3EZQ3S2CFGPANCNFSM4VNZUVUA .

zhongduo commented 3 years ago

/unhold