Remove WebhookConfig.Headers in favour of WebhookConfig.HTTP.HTTPHeaders #122
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #122 +/- ##
=======================================
Coverage 99.98% 99.98%
=======================================
Files 78 78
Lines 6437 6439 +2
=======================================
+ Hits 6436 6438 +2
Misses 1 1 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Chris Bygrave <chris.bygrave@kaleido.io>
1422330 to
0233847
Compare
| @@ -34,7 +34,6 @@ import ( | |||
| type WebhookConfig struct { | |||
| URL *string `ffstruct:"whconfig" json:"url,omitempty"` | |||
| Method *string `ffstruct:"whconfig" json:"method,omitempty"` | |||
| Headers map[string]string `ffstruct:"whconfig" json:"headers,omitempty"` | |||
There was a problem hiding this comment.
This would have to feed its way into a release note as it's a breaking change, I wonder if we want a deprecated field instead?
There was a problem hiding this comment.
I suggest at the stage of lifeycle we're at with this function we're ok with the release notes item.
I can include that cutting a release on the common library.
pkg/eventstreams/webhooks.go
Outdated
| if w.spec.HTTP != nil { | ||
| for h, v := range w.spec.HTTP.HTTPHeaders { | ||
| if vs, ok := v.(string); ok { | ||
| req.Header.Set(h, vs) | ||
| } | ||
| } |
There was a problem hiding this comment.
I think we just remove this. They get added as part of the init of the HTTP Config so this will result in double work.
There was a problem hiding this comment.
There was a problem hiding this comment.
Good point - thanks
Signed-off-by: Chris Bygrave <chris.bygrave@kaleido.io>
WebhookConfig was recently introduced with a top-level
headersproperty. However it also contains anhttpproperty - a standardffrestry.HTTPConfigobject which already provides the facility for defining HTTP headers.This PR removes the top-level
headersproperty, and updates the web hook action to useHTTP.HTTPHeaderson the request instead.