Skip to content

Accept v0.1 and v0.2 cloud events. Adding UTs.#40

Merged
Harwayne merged 3 commits into
Harwayne:broker-new-modelfrom
nachocano:cloud_events
Mar 14, 2019
Merged

Accept v0.1 and v0.2 cloud events. Adding UTs.#40
Harwayne merged 3 commits into
Harwayne:broker-new-modelfrom
nachocano:cloud_events

Conversation

@nachocano
Copy link
Copy Markdown

Updating initClient as well, removing unnecessary paging.

Proposed Changes

  • Change to accept cloud events v0.1 and v0.2.
  • Added UTs.
  • Removing unnecessary paging from initClient method.

Release Note

NONE

Updating initClient as well, removing unnecessary paging.
@nachocano
Copy link
Copy Markdown
Author

@Harwayne Done!

@nachocano
Copy link
Copy Markdown
Author

@Harwayne @vaikas-google

BTW, sorry to keep on bothering with the same thing :) I know we discussed it yesterday, but I'm still not completely convinced on the idea of non-prefix match for sources for a first cut.
I think it is very easy to add a basic prefix matching for sources, which will make the whole filtering functionality (type and source) usable from day 1. If we wait until a decision on the filtering language is made (which according to my short tenure here might take months), then consumer functions will get bombarded with events from repos they might not want.

The code change I'm suggesting is just a single line:

strings.HasPrefix(cloudEventSource, filterSource) instead of cloudEventSource == filterSource.

This will cover the exact matching as well as "startsWith" kind of thing.
A user can then create a trigger for receiving pull requests only from her own repo.

With this we can actually test if people care about sources at all, which I think they will.
IMO we should try to include this for the big PR. I can do it. If you guys don't agree, I think it would still be nice to have it for 0.5.

Comment thread pkg/broker/receiver_test.go Outdated
@Harwayne Harwayne merged commit 0d169db into Harwayne:broker-new-model Mar 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants