Test-infra: Tide is serving bad JSON data to Deck.

[cjwagner]

ref: https://github.com/kubeflow/community/issues/20

Tide is improperly serializing its pool data. Here is a sample:

{"Context":"coverage/coveralls",
"Description":"Coverage increased (+0.1%!)(MISSING) to 27.599%!"(MISSING),
"State":"SUCCESS"}

Even though this is not valid JSON, json.Marshal does not throw an error when emitting this text (?!!) so tide serves it, thinking that it is valid.
https://github.com/kubernetes/test-infra/blob/ef5978825db04c5c161574ad7395831850ab92df/prow/tide/tide.go#L381-L386

I've been able to reproduce the behavior locally in a unit test and it seems likejson.Marshaldoesn't behave well when marshaling github.com/shurcooL/githubql.String types with values containing "%" symbols.

I think we will need to make a struct that doesn't use githubql.Strings and marshal/unmarshal slices of that struct instead of the []PullRequest fields in tide.Pool.
Alternatively we could try a different JSON parsers...

Tide's main sync loop is unaffected by this so testing and merging is working as expected, but the "Tide Status" page is not going to show any data until we resolve this or disable tide for the repos that are using the "coverage/coveralls" context.

/kind bug
/area prow
cc @BenTheElder @kargakis @stevekuznetsov @spxtr

[cjwagner]

/assign

[BenTheElder]

we could also escape these strings or use another format. protos anyone? we can finally gRPC the prow components :-)

[stevekuznetsov]

Or define custom marshalling functions for the PullRequest struct?

[cjwagner]

Or define custom marshalling functions for the PullRequest struct?

That sounds like the best option to me.

It seems like there might be a bug in the encoding/json package? Even if there is a custom marshalling function in githubql that is misbehaving, it shouldn't be possible for json.Marshal to emit invalid JSON without throwing an error...

[stevekuznetsov]

Right, that may need to be an upstream issue :|

[spxtr]

If you're confident that it's a bug in githubql then feel free to open an issue either there or in shurcooL/graphql. shurcooL has been quick to fix bugs in the past. My guess whenever there is an error involving percent signs in strings is that someone is passing them to a printf-like function somewhere.

[BenTheElder]

+1, definitely looks like bad usage of fmt
Also: Hi Joe, don't you have lab work to do? 😄

On Fri, Mar 2, 2018 at 10:46 AM Joe Finney [e-mail] wrote:

If you're confident that it's a bug in githubql then feel free to open an
issue either there or in shurcooL/graphql. shurcooL has been quick to fix
bugs in the past. My guess whenever there is an error involving percent
signs in strings is that someone is passing them to a printf-like
function somewhere.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/test-infra/issues/7091#issuecomment-370015551,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA4Bq_PSr6FVOUYL-v-TNJHAfIg18JZkks5taZOLgaJpZM4SZTyD
.

[cjwagner]

Yeah I suspected a misused printf as well, but didn't see anything obvious in the githubql or graphql repos. I'm not confident that there is a bug in graphql. I'm more confident that there is some sort of bug in json.Marshal given that it shouldn't ever emit invalid JSON without an error.

[BenTheElder]

I scanned github.com/shurcooL in our vendor/ for fmt usage, looks like only fmt.Errorf

[cjwagner]

Well I feel very stupid right now.
See the fmt.Fprintf(w, string(b)) in the code snippet in the issue body...
/shrug
/cole-is-bad-at-reading

[BenTheElder]

LOL we all missed that :joy:

[cjwagner]

1 character fix incoming.

[spxtr]

Hah! That was probably my bad.