Go-github: Remove boilerplate for receiving webhooks

[apelisse]

I have two goals:

• Remove the amount of boilerplate you have to write if you want to create a small app listening to webhooks,
• Hide the event interface{} cast to actual type from the user.

I'm proposing something like this:

whl := NewWebHookListener("my-github-secret")

// Mimic http.Handler
type MyCommitCommentEventHandler struct{}
func (MyCommitCommentEventHandler) ProcessHook(w http.ResponseWriter, event CommitCommentEvent) {
    fmt.Println("Received CommitCommentEvent:", event)
    w.Write([]byte("OK"))
}

// The following two functions mimic net/http Handle() and HandleFunc()
whl.CommitCommentEventHandle(MyCommitCommentEventHandler{})
whl.CommitCommentEventHandleFunc(func(w http.ResponseWriter, event CommitCommentEvent) {
    fmt.Println("Received CommitCommentEvent:", event)
    w.Write([]byte("OK"))
})

// Add *Handle and *HandleFunc for each type of events.

log.Fatal(whl.ListenAndProcess(":8080"))

Is that something you would be interested in merging in your tree?

Thanks!

[dmitshur]

I feel like I've seen something very similar proposed somewhere, but I can't find it now.

[apelisse]

This is sort of a follow-up of #427

@gmlewis already mentioned that this is done one way or another in different places. I think it would make sense to have a common way to do it (while removing the boilerplate code).

I could give a shot at implementing this, it should be quite easy, but I want to know if it's a dead-end before.

[willnorris]

My initial gut reaction is that this is a bit beyond the scope of what go-github should be. It's designed to strictly implement the GitHub API with as little interpretation or opinion as possible. How the data from GitHub is handled or processed is the responsibility of the caller.

If enough people feel strongly that this is generally useful enough to include, then I would probably propose putting it into a separate "github.com/google/go-github/webhook" package. If we move in that direction, then arguably the code from #427 should be moved there as well. At that point, the main go-github/github package remains the strict implementation of the API and go-github/* (excluding the existing "tests" and "examples") become helpers for working with the data for various use cases.

[dmitshur]

I'd also like to point out the approach @bradleyfalzon took in #455, which is to place that higher level helper code into a separate repository.

I'm not familiar with the installations service so I can't review it, but if someone else here can and it looks good and more people can find it useful, then I think we should mention it in the README.

Perhaps that's a good approach to take. Just a thought.

[apelisse]

I like the proposal of having it in a different package, and respect the idea that go-github/github should be a pure implementation of the API.

If it ends-up in a different repository, then:

• Where would it live and with what name? How do people discover it?
• It would be very strongly coupled to go-github repository

[dmitshur]

Good questions. Just to clarify, I didn't meant it has to live in a separate repo, just that it might be a good idea to prototype it there first and then decide to bring it into this repo, or link to it, etc.

That way you're not blocked on getting started and can do some prototyping/iterations more quickly to learn what works well, and get to a better design before freezing it.

Where would it live and with what name? How do people discover it?

Up to you on the naming, I'd imagine something under your github username. We can link to it from README (and package doc, they're in sync). Maybe there are better ideas?

It would be very strongly coupled to go-github repository

More reasons to consider bringing it in when it's ready. But I don't think it's a problem, you'd just have update it if there are any breaking changes in this package. I don't think vendoring is a viable solution.

[parkr]

For reference, I have done this with my project auto-reply. It takes in the request, gathers N handlers for each event type, reads and unmarshals the request body into the proper webhook payload struct, and fires each handler in a separate goroutine with the payload. It requires a small amount of boiler-plate for each new handler as the payload is passed in as interface{} but I have found it very helpful to make this project extensible. If there were a way for this project to just provide the payload handling logic, I would be a happy camper. But then we run into questions of how the event handlers should be fired – asynchronously or within the same goroutine as the request? If we go with the latter but I want the former, I guess I just end up with handlers that are one-liners like go doMyStuffBehindTheScenes().

To me it feels like there isn't a solid motivation for creating it. All it would do is ease the boilerplate those of us setting up webhook servers need to copy around. :shrug:

[apelisse]

Yeah, I'm inclined to close this issue after all ...