Add an Event interface

[cco3]

Right now Event.ParsePayload returns interface{}, but it would be nice if it returned a more specific interface with the common fields of events.

It looks like using the Event struct fully is deprecated, and now it is just used to load up with the payload before calling parse.

I propose replacing the Event struct with an Event interface and then making ParsePayload a stand alone function rather than a receiver function.

Does this sound reasonable?

[gmlewis]

Hmmm... this sounds intriguing... can you please elaborate and give us an idea of how your proposed interface and stand-alone function would be used?

Maybe you could also give a list of pros/cons to the old way and new way of processing events?

[cco3]

e, err := events.ParsePayload(myBytes)
log.Printf("Basic info %s %s\n", *e.GetId(), *e.GetRepo().Name)

Right now, even to look at the basic fields shared by (nearly?) all events, you have to construct a massive switch statement that handles every different event type.

Something like:

e := &Event{
  RawPayload: myBytes,
  Type: myType,
}
e.ParsePayload()
switch e := e.(type) {
case *github.CommitCommentEvent:
  log.Printf("Basic info %s %s\n", *e.Id, *e.Repo.Name)
case *github.CreateEvent:
  log.Printf("Basic info %s %s\n", *e.Id, *e.Repo.Name)
// Continues for ~30 more cases
}

Additionally, some of the defined structs don't even have the basic info like ID (I think PushEvent is one that doesn't have ID). This interface would guarantee that all event structs contain a common core of shared fields.

[gmlewis]

I like the idea, but I also know from first-hand experience that there are some non-trivial systems built using the current existing method.

I think we could make the two methods co-exist.

Normally we don't like to provide two ways of doing things as that increases the maintenance costs of this repo, and maybe now that we have semver versioning, we could switch directly over to the new mechanism (and remove the old), but that might prevent some people from upgrading due to the extra cost of migration.

So I'm leaning toward implementing the two mechanisms and having them co-exist.

I would be interested in hearing the thoughts of @willnorris, @gauntface, @rspier, and @juliaferraioli (and anyone else) if they have strong opinions one way or another.

[gauntface]

Does go have the notion of deprecation notations for their docs? If so we could support both methods and add a deprecation not for the old method.

Regarding the actual proposal - I'm in favor of what @cco3 is proposing.

[rspier]

I'd like to learn more about the proposed shared fields. The events themselves don't have some of these fields, so it's not clear if the plan is to synthesize them. There aren't very many (any?) that are shared across all the event types.

There are some disadvantages to returning an interface type -- for example, you can't directly access any of the members. (Of course, this gets into whether or not you like accessors.)

Also, is this really a general problem? Or is it specific to what we're trying to solve? I don't see a giant switch statement full of type assertions as a huge problem. go generate might be useful to help if you don't want to hand maintain it.

[cco3]

Sorry, I should have given an update yesterday...apparently the data from the fetch api and the data from the webhooks are substantially different, so the commonality of fields is less than I was thinking. In the fetch api, they all seem to have a lot of common fields.

Anyway, at this point I'm fine with closing this unless anyone thinks there's something more here worth discussing.

[gmlewis]

Closing inactive issue for lack of activity/interest.

[wlynch]

I'm interested in revisiting this - I found myself wanting something like this today 👀
Things probably changed a bit since this was last looked at.

GitHub now documents a a set of common properties that every event should have that I'm pretty sure every event message type in go-github conforms to, so I think we can replace ParseWebhooks's interface{} with something like:

go-github/github/messages.go

Line 295 in 15b20d3

func ParseWebHook(messageType string, payload []byte) (interface{}, error) {

type WebhookEvent interface{
  GetAction() string
  GetInstallation() *Installation
  GetOrg() *Organization
  GetRepo() *Repository
  GetSender() *User
} 

The specific use case I'm trying to target (which I think was inline with what @cco3 wanted as well) is middleware that extracts common properties (e.g. repo name, etc.) before handing off the event to type-specific handlers. First application I had in mind was annotating a context logger, but also seems also useful for other middleware operations like metrics. e.g.

event, _ := github.ParseWebHook(github.WebHookType(r), payload)
log := zap.With(
  zap.String("installation", event.GetInstallation().GetID()),
  zap.String("repo", event.GetRepo().GetName(),
  ...
)
switch event := event.(type) {
  case *github.PullRequestEvent:
    handlePullRequest(log, event)
  ...
}

I'm pretty sure this change would generally be backwards compatible as well, since anything that was accepting an interface{} before would still be satisfied by a more specific interface.

Happy to work on this, let me know what you think!

[gmlewis]

I'm pretty sure this change would generally be backwards compatible as well, since anything that was accepting an interface{} before would still be satisfied by a more specific interface.

This sounds very interesting, @wlynch !

How about putting together a PR and seeing if you can implement your ideas in a backward-compatible way so as to not affect existing systems based on the previous mechanism... and we'll take it from there?

[wlynch]

Was definitely optimistic here - turns out the "common properties" are only mostly common. 😓

Some are obvious in hindsight, but here are a few themes I saw -

• Organization events don't act on a repository, so there is no repository field.
• Some events like RepositoryImport cannot be performed by an installation, so there is no installation field
• Installation events can act on multiple repositories at once, so it's repositories instead of repository
• We're inconsistent with naming in some places - Repo vs Repository, Org vs Organization.

It did result in a nice spot check to see if we were missing any fields though! Went ahead and created #2384 for fields I noticed. Otherwise we can close this out again.

[gmlewis]

Thank you for the investigation and PR, @wlynch !

I don't mind minor breaking API changes for the sake of consistency... For example if we were to start consistently using the shorter forms of Org and Repo... but I'll take a look at the PR later today.