Enable OPTIONS and CloudEvent Webhook headers #5542
Conversation
knative-prow-robot
added
the
do-not-merge/hold
knative-prow-robot
added
size/M
Codecov Report
@@ Coverage Diff @@
## main #5542 +/- ##
==========================================
+ Coverage 82.73% 82.76% +0.03%
==========================================
Files 200 200
Lines 6243 6256 +13
==========================================
+ Hits 5165 5178 +13
Misses 748 748
Partials 330 330
Continue to review full report at Codecov.
|
|
It looks like I'm going to need to do some surgery on |
knative-prow-robot
added
the
area/test-and-release
evankanderson
force-pushed
the
you-leave-me-no-options
branch
from
f1b0374
to
a8319d8
Compare
evankanderson
force-pushed
the
you-leave-me-no-options
branch
from
a8319d8
to
40924fd
Compare
|
/hold cancel |
knative-prow-robot
removed
the
do-not-merge/hold
| @@ -101,7 +101,14 @@ func (h *Handler) Start(ctx context.Context) error { | |||
| } | |||
|
|
|||
| func (h *Handler) ServeHTTP(writer http.ResponseWriter, request *http.Request) { | |||
| writer.Header().Set("Allow", "POST, OPTIONS") | |||
There was a problem hiding this comment.
should the spec say something about OPTIONS ? https://github.com/knative/specs/blob/main/specs/eventing/broker.md#ingress
There was a problem hiding this comment.
http://github.com/knative/specs/pull/25, data-plane.md, line 88.
There was a problem hiding this comment.
(I started working ahead of that commit landing)
pkg/channel/message_receiver.go
Outdated
| @@ -136,6 +136,13 @@ func (r *MessageReceiver) Start(ctx context.Context) error { | |||
| } | |||
|
|
|||
| func (r *MessageReceiver) ServeHTTP(response nethttp.ResponseWriter, request *nethttp.Request) { | |||
| response.Header().Set("Allow", "POST") | |||
There was a problem hiding this comment.
Header().Set("Allow", "POST, OPTIONS")
? or why is that different here
There was a problem hiding this comment.
Because I'm an idiot.
Thanks!
(And, if I'd gotten all my e2e testing working, I'd have noticed this.
There was a problem hiding this comment.
It actually turns out that my tests are all wrong, because receiverFunc is not called at all for OPTIONS requests.
Fixed.
|
Looks like you need the latest go-licenses to fix the filemode changes |
|
@evankanderson any updates on this PR ? |
|
Fixed the formatting bugs; not sure how they got in there. |
|
The following is the coverage report on the affected files.
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: evankanderson, matzew 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 |
Fixes #3092
I'm still working on e2e testing / getting the e2e tests to run at all in my environment, but I wanted to put this out there.
Proposed Changes
Allowheader on 405.Pre-review Checklist
/hold work-in-progress
Release Note
Docs
Should not have user-visible impact. Does have specs-visible impact, aligns with knative/specs#25