Support issue & PR comment events in the webhook service.

[chhsia0]

The goal of this issue is to discuss technical details about how to implement issue & PR comment events.

In some drivers, e.g., github, we might need a context for its webhook service to reach out to the API endpoint to retrieve additional information. For example, when processing an issue comment event from a pull request, we might need to issue another http request to get the pull request information which is not in the event payload.

So I'm suggesting to change the signature of the Parse function to:

Parse(ctx context.Context, req *http.Request, fn SecretFunc) (Webhook, error)

Since this is a breaking change, I'd like to raise some discussion first.

[chhsia0]

@bradrydzewski Could you provide some feedback about this? Thanks.

[chhsia0]

An alternative to avoid the breaking change is to use req.Context() as the context to reach out to the API endpoint. However that means if the caller want to interrupt the call, they needs to set the context object in the request first and use it to control the Parse call, so it's less obvious. WDYT?

[bradrydzewski]

I really dislike the way we parse webhooks today. I would like to add methods to more closely match the go-github library where parse and verify are separate methods, with no awkward callback required.

I believe this can be done by adding new methods, instead of breaking existing methods.

For example, when processing an issue comment event from a pull request, we might need to issue another http request to get the pull request information which is not in the event payload.

If you need to augment the webhook with additional information, I am of the opinion that this should be handled separately, outside of the webhook parsing code. For example, in Drone, if the commit sha is empty for a tag creation webhook, we make an API call to retrieve additional information about the tag to fill in the blanks. This extra logic is handled by Drone, not by go-scm.

If this extra logic ends up being common across multiple projects, I could see us having some sort of optional helper package as part of this library. For example:

package normalize

// Webhook normalizes the webhook response and attempts to populate
// missing information not included in the webhook payload.
func Webhook(ctx context.Context, client scm.Client, webhook *scm.Webhook) error {
  if v, ok := webhook.(scm.TagHook); ok {
    if v.Ref.Commit == "" {
      // make API call to get Tag and set the commit sha
      v.Ref.Commit = ...
    }
  }

  return nil
}

Also, speaking in the context of Drone, when we parse a webhook we do not know the repository name yet, nor do we know who owns the repository. If we cannot associate the webhook with a repository and, more importantly, a user, we do not have an oauth2 token that we can use to make additional API calls. This is why we do not make extra API calls in the parse method.

[chhsia0]

The idea of a normalize package is neat. However, either the library requires the user to always use the package, or the webhook struct needs to have some indicator for the user to know when to call it.

For the latter, I just came up with a couple options:

1. Change PullRequestHook (which seems okay because it is not supported by and service yet):

type PullRequestHook struct {
    ...
    PullRequest       *PullRequest
    PullRequestNumber int
}

Or

type PullRequestHook struct {
    ...
    PullRequest PullRequest
    Normalized  bool
}

2. Add a Normalized field in PullRequest:

type PullRequest struct {
    Number     int
    Normalized bool
    ...
}

3. Add an IsNormalized() function to the WebhookService interface, so each driver does its own check.
4. Add an IsNormalized() receiver function to Webhook or PullRequest that do the same check for all drivers (e.g., check if Sha is set).
5. Add another function that returns (*scm.Webhook, bool, error) to the WebhookService interface, where the second return value indicates if the webhook is normalized. If false is returned, then only certain documented fields in a particular webhook struct is valid (e.g., PullRequestCommentWebhook.PullRequest.Number). This could be made to align with go-github where we don't do signature validation in this new function but store the payload in the Webhook struct.

After listing all possibilities, I'm now a bit leaning towards option 5. WDYT?

[bradrydzewski]

the library requires the user to always use the package

I prefer that the user always invokes the normalizer package if needed. Most people will never need this package, but if they do, it can be opt-in.

[chhsia0]

What I mean is that, say any PullRequestCommentWebhook from github always requires a normalization, but no normalization is needed for gitlab. Since the user shouldn't care about what the underlying driver is, if we present this normalization as a utility function, that means the user must always opt-in, or they will get an incomplete PullRequestCommentWebhook, unless the library can provide some hint to the user about when they should do the normalization.

[bradrydzewski]

if we present this normalization as a utility function, that means the user must always opt-in

Correct, we would require a user to always opt-in if they want to receive the pull request comment webhook for GitHub. This will not impact any of our existing users since this webhook is currently a noop.

I absolutely understand where you are coming from, but I think this just comes down to personal preference. I am not sure what sort of long term solution is best. I am not sure I understand the scope of this problem well enough to design a solution today that will work with all providers, or with new webhooks or providers we implement tomorrow. So I would opt for a short term solution that does not make any breaking changes, nor does it introduce changes to the core API that we may need to change or break in the future.

I feel like over time we will have a better understanding of the problem, and can revisit how to design a more permanent solution.