support yaml payloads for generic webhooks #10031
Conversation
|
@csrwng ptal. embarassingly our doc suggests a yaml payload :( @openshift/api-review this isn't a huge api change, but ptal/sign-off. |
|
[test] |
bparees
changed the title
|
flake #9959 |
| @@ -65,8 +66,13 @@ func (p *WebHookPlugin) Extract(buildCfg *api.BuildConfig, secret, path string, | |||
| } | |||
|
|
|||
| var data api.GenericWebHookEvent | |||
| body, err = yaml.ToJSON(body) | |||
There was a problem hiding this comment.
shouldn't we skip this conversion if the contentType == "application/json" ?
There was a problem hiding this comment.
yaml.ToJSON no-ops if the content is json.
so this way we will "work" even if they screw up the content-type header, and no real additional cost.
There was a problem hiding this comment.
it does, but the error is really confusing ("yaml: unmarshal errors...")... we should only do it when the content-type indicates it is yaml
There was a problem hiding this comment.
@liggitt ?
func ToJSON(data []byte) ([]byte, error) {
if hasJSONPrefix(data) {
return data, nil
}
return yaml.YAMLToJSON(data)
}
Basically this code now says "if you tell us it's yaml or json, we will convert it to json and then process it as json. If it was originally json, then we will no-op the conversion"
where does the error come in?
There was a problem hiding this comment.
this should not return a yaml unmarshal error:
curl -X POST -H "Content-Type: application/json" --data "invalid data"
There was a problem hiding this comment.
it's a log level 4 error that doesn't even go back to the user, but ok.
There was a problem hiding this comment.
(And it means if someone specifies content-type json but supplies yaml, the payload will now be ignored because it'll catch an error)
|
LGTM |
|
[test] On Tue, Jul 26, 2016 at 10:16 AM, OpenShift Bot [email protected]
Ben Parees | OpenShift |
|
@liggitt updated. |
| if err = json.Unmarshal(body, &data); err != nil { | ||
| glog.V(4).Infof("Error unmarshaling json %v, but continuing", err) | ||
| glog.V(4).Infof("Error unmarshalling payload %v, but continuing with build", err) |
There was a problem hiding this comment.
huh... I didn't realize we continued in cases where the body was invalid. I could maybe see doing that if the body was empty, but it seems wrong to just ignore malformed content, especially if the data could change the behavior of the launched build
There was a problem hiding this comment.
i don't remember exactly why we made that choice, but i think we're mostly stuck with it now.
There was a problem hiding this comment.
it's also unfortunate that all other content types get ignored and "successfully" do a generic build
|
changes look fine, can we add a test that submits yaml and makes sure the payload actually takes effect? |
|
@liggitt test added |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6955/) (Image: devenv-rhel7_4675) |
|
Evaluated for origin merge up to 10ba6bd |
|
Evaluated for origin test up to 10ba6bd |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6909/) |
No description provided.