Implement receive_quality for ticket trackers

[pbrisbin]

• Pivotal/Lighthouse/GitHub
• Adds Faraday Middlewares to DRY up HTTP a little bit
• GH requires a new project config option (open to ideas here)

This is far enough along to move over to CC and start wiring in the
QualityTicketsController.

[brynary]

I'm not sure the "custom middleware" are a clear win. They add some conceptual complexity, and the DRY benefit doesn't seem so great. Also, the JSON middleware appears to be about modifying the request, whereas the XML middleware is about parsing the response -- which is a bit confusing.

Maybe simpler/clearer would be to add a http_post_json method which set's the Content-Type and serializes the params and leaving?

[brynary]

Not sure if this check for a 2XX response code got lost

[pbrisbin]

Now that we raise and handle errors in a more sensible fashion, I felt it was no longer needed. Disagree?

[brynary]

Hmm, well here's my question: If we get back a non-200, which is not valid JSON, will this transform into a JSON::ParseError? That's the only thing.

[pbrisbin]

Without the check, an error of some sort will be thrown, probably No method or Can't coerce String to Integer as res.body["foo"] fails. The error will be handled, we'll retry, log and increment statsd. If the problem persists, a nil is returned and the user is redirected with an error message.

With the check, a nil is silently and immediately returned and the user is redirected with an error message.

I don't see a downside in letting it error to get the metrics/logging on it.

[pbrisbin]

I'm not sure the "custom middleware" are a clear win. They add some conceptual complexity, and the DRY benefit doesn't seem so great.

Hmm. I guess I don't see it as conceptually complex and the JSON handling being peppered across 4 service classes felt really noisy to me. I like the declarative style: "This service interacts with a JSON API", then just call http methods and everything "Just works".

Also, the JSON middleware appears to be about modifying the request, whereas the XML middleware is about parsing the response -- which is a bit confusing.

To me they're meant as black boxes. Interacting with a JSON API requires we send and parse JSON, interacting with an XML API requires we just parse the response. Conceptually similar, only differing in implementation.

Maybe simpler/clearer would be to add a http_post_json method which set's the Content-Type and serializes the params and leaving?

I could do that, but to me it feels like little gain at that point. I'd almost rather go completely back and just live with the duplication for now -- I'll do that now to see how we feel about the net result.

[brynary]

Sounds good.

[brynary]

Oops.

[pbrisbin]

OK, done -- also moved some things back to reduce the diff. We can always refactor separately later.

[brynary]

This line makes me a bit nervous from a security standpoint, but I can't immediately see how to break it (without looking at e.g. Faraday code). However it raises an overall concern of validating supplied config data. Probably should just have a separate Trello story for "Validate service configs". For things like tokens, and this GH project name, we can use format validations.

[pbrisbin]

For my own education, assuming Faraday is as dumb as possible, what value here would break things and how?

[brynary]

I'm not sure there is a value that would break it. If I was trying to hack it, I'd try things like sending in a string with a null byte, hoping that something further down the C stack allows me to inject a new URL

[pbrisbin]

Ah I see. OK, I'll create a Trello card at the top of Backlog, please prioritize it as you see fit.

[pbrisbin]

BTW, I'm fine with the PR as-is (duplication, etc). I'll keep my eyes open for refactoring opportunities further down the line when we have more services in play.