Skip to content

Enable passing CloudEvents headers ('CE-*')#180

Closed
evankanderson wants to merge 1 commit into
knative:masterfrom
evankanderson:sqeaky-clean
Closed

Enable passing CloudEvents headers ('CE-*')#180
evankanderson wants to merge 1 commit into
knative:masterfrom
evankanderson:sqeaky-clean

Conversation

@evankanderson
Copy link
Copy Markdown
Member

Also cleans up some go vet warnings.

THIS IS NOT TESTED. Mostly because we don't seem to have any test cases yet for these buses. It seems like it would be useful to have a small test harness that accepts a set of HTTP requests and sends them to the bus then verifies that the correct messages were received by standing up a listening HTTP server and looping back from the bus to the handler.

Works towards #53

Proposed Changes

  • Add CE- headers to the whitelist on the two buses that use a whitelist for handling messages. Long term, I'd like to see the buses move towards a blacklist model in terms of header passing, both in light of Make extension attributes siblings to our attributes cloudevents/spec#225 and to enable other development and experimentation without needing to rebuild the controller.
  • Added docstrings wherever they were missing, or made methods which were not used outside the package private in the gcppubsub case.

<--
/assign @scothis
/assign @ericbottard
-->

@google-prow-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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

@google-prow-robot google-prow-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 Jul 12, 2018
@scothis
Copy link
Copy Markdown
Contributor

scothis commented Jul 12, 2018

@evankanderson I started an effort to centralize the http bit for the buses over at #175. I added support for the common ce-* headers over there, but not via a prefix which we'll need to handle ce extensions.

I can port your change on top of that PR if you'd like to close this one.

@evankanderson
Copy link
Copy Markdown
Member Author

That sounds fine with me; what's the ETA on that PR?

I did this as part of trying to thread cloud events though the whole system.

What's the rationale behind removing most headers in the bus? It seems like 2 of 3 implementations do this, but it seems hostile to extension (as we're seeing with cloud events).

@scothis
Copy link
Copy Markdown
Contributor

scothis commented Jul 13, 2018

/hold

@google-prow-robot google-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 13, 2018
@evankanderson
Copy link
Copy Markdown
Member Author

Closing in favor of #175

matzew pushed a commit to matzew/eventing that referenced this pull request Sep 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants