Test-infra: yuks: jokes contain invalid characters for GitHub comments

[stevekuznetsov]

{"author":"Fedosin","component":"hook","event-GUID":"88985c70-dbb9-11e9-87c2-21e6a3d2747f","event-type":"issue_comment","file":"prow/plugins/yuks/yuks.go:128","func":"k8s.io/test-infra/prow/plugins/yuks.handle","level":"error","msg":"joke contains invalid characters: The first time I got a universal remote control I thought to myself, \"This changes everything\"","org":"openshift","plugin":"yuks","pr":1127,"repo":"machine-config-operator","time":"2019-09-20T15:15:52Z","url":"https://github.com/openshift/machine-config-operator/pull/1127#issuecomment-533595262"}
{"author":"Fedosin","component":"hook","event-GUID":"88985c70-dbb9-11e9-87c2-21e6a3d2747f","event-type":"issue_comment","file":"prow/plugins/yuks/yuks.go:128","func":"k8s.io/test-infra/prow/plugins/yuks.handle","level":"error","msg":"joke contains invalid characters: My cat was just sick on the carpet, I don’t think it’s feline well.","org":"openshift","plugin":"yuks","pr":1127,"repo":"machine-config-operator","time":"2019-09-20T15:15:52Z","url":"https://github.com/openshift/machine-config-operator/pull/1127#issuecomment-533595262"}
{"author":"Fedosin","component":"hook","event-GUID":"88985c70-dbb9-11e9-87c2-21e6a3d2747f","event-type":"issue_comment","file":"prow/plugins/yuks/yuks.go:128","func":"k8s.io/test-infra/prow/plugins/yuks.handle","level":"error","msg":"joke contains invalid characters: What did the late tomato say to the early tomato? I’ll ketch up","org":"openshift","plugin":"yuks","pr":1127,"repo":"machine-config-operator","time":"2019-09-20T15:15:52Z","url":"https://github.com/openshift/machine-config-operator/pull/1127#issuecomment-533595262"}

/cc @cjwagner @fejta
/priority critical-urgen

[k8s-ci-robot]

@stevekuznetsov: The label(s) priority/critical-urgen cannot be applied. These labels are supported: api-review, community/discussion, community/maintenance, community/question, cuj/build-train-deploy, cuj/multi-user, platform/aws, platform/azure, platform/gcp, platform/minikube, platform/other

In response to this:

{"author":"Fedosin","component":"hook","event-GUID":"88985c70-dbb9-11e9-87c2-21e6a3d2747f","event-type":"issue_comment","file":"prow/plugins/yuks/yuks.go:128","func":"k8s.io/test-infra/prow/plugins/yuks.handle","level":"error","msg":"joke contains invalid characters: The first time I got a universal remote control I thought to myself, \"This changes everything\"","org":"openshift","plugin":"yuks","pr":1127,"repo":"machine-config-operator","time":"2019-09-20T15:15:52Z","url":"https://github.com/openshift/machine-config-operator/pull/1127#issuecomment-533595262"}
{"author":"Fedosin","component":"hook","event-GUID":"88985c70-dbb9-11e9-87c2-21e6a3d2747f","event-type":"issue_comment","file":"prow/plugins/yuks/yuks.go:128","func":"k8s.io/test-infra/prow/plugins/yuks.handle","level":"error","msg":"joke contains invalid characters: My cat was just sick on the carpet, I don’t think it’s feline well.","org":"openshift","plugin":"yuks","pr":1127,"repo":"machine-config-operator","time":"2019-09-20T15:15:52Z","url":"https://github.com/openshift/machine-config-operator/pull/1127#issuecomment-533595262"}
{"author":"Fedosin","component":"hook","event-GUID":"88985c70-dbb9-11e9-87c2-21e6a3d2747f","event-type":"issue_comment","file":"prow/plugins/yuks/yuks.go:128","func":"k8s.io/test-infra/prow/plugins/yuks.handle","level":"error","msg":"joke contains invalid characters: What did the late tomato say to the early tomato? I’ll ketch up","org":"openshift","plugin":"yuks","pr":1127,"repo":"machine-config-operator","time":"2019-09-20T15:15:52Z","url":"https://github.com/openshift/machine-config-operator/pull/1127#issuecomment-533595262"}

/cc @cjwagner @fejta
/priority critical-urgen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[stevekuznetsov]

/priority critical-urgent

[Biwwie]

Debugging:

Examples 1: Regex does not match \"
Examples 2 + 3 : Regex only includes "Single Quote" but not "Right Single Quotation Mark"

Regex: https://github.com/kubernetes/test-infra/blob/master/prow/plugins/yuks/yuks.go#L35

[Biwwie]

Option 1: Just remove the regex
Option 2: Update regex to accept the additional characters

[fejta]

Option 1 is a safeguard. Please don't remove the regex

[hvaara]

@Biwwie Have you already started working on this, or would you like to? If not, I'd love to have a look myself 😄
@fejta Can you please explain what the regex safeguards against?

[fejta]

Safeguard against arbitrary data. A joke that contains only https://github.com/kubernetes/test-infra/blob/aec361073b4ad2290e8bffd552a68012938a9775/prow/plugins/yuks/yuks.go#L35 characters is much less likely to cause problems downstream than allowing anything.

[hvaara]

Would it make sense to sample the api for some jokes and expand the regex with the character set that is seen given it is reasonable? What about introducing retry logic in case the regex match fails for whatever reason?

[fejta]

SGTM, that's essentially what we did originally. Not sure if I failed to sample enough or if the set has drifted over time.

[fejta]

Retry also sgtm

[Biwwie]

@hvaara all yours, I just did some light debugging.

Regex: I think our regex should match Githubs comment's supported character set. Looks like the both of you have agreed on the less intrusive approach and to expand the character set in some manner. SGTM.

Retry: I don't see the value here, if you only retry once you potentially could still fail which would result in a future Issue/Regex update.

[fejta]

Retries are extremely effective. If there's a 20% of finding a joke with a bad character (extremely bad), five attempts will regardless provide users with a successful joke 99.97% of the time instead of 80%.

[hvaara]

Thanks for the comments everyone! I'll take a look 😃
/assign
/lifecycle active