Inconsistency in representing events in HookInput and Hook

[chhsia0]

Right now, hooks are created with parameters specified by HookInput, which contains a HookEvents to indicate which abstract event should be listened to. But the Hook struct returned by FindHook() contains Events []string, which is a list of native event names. This makes it hard to implement a "reconciliation" logic, where the program needs to check if an existing webhook matches a HookInput to decide if it should recreate the webhook. The workaround is to only use HookInput.NativeEvents and don't rely on HookInput.Events at all.

It seems reasonable to change Hook.Events from []string to HookEvents, and introduce a HookEvents.NativeEvents field to make the API more consistent. However, these are all breaking changes.

Would it make sense to deprecate HookInput.Events, HookInput.NativeEvents and Hook.Events, and instead having a new field name for HookEvents in both HookInput and Hook?

[bradrydzewski]

I do not believe the webhook events can be fully abstracted — they are not consistently implemented across providers — which is why we return the native event types. Native events are required for creating hooks with provider-specific events that are not abstracted by or supported by this library. I consider HookEvents to be a convenient option to create all hooks that are supported by this library, but not a viable replacement for NativeEvents.

The reason I did not use HookEvents when returning the Hook structure is because I did not believe it would be possible to accurately perform this mapping. For example, what happens if the create event is enabled but the delete event is disabled in GitHub? What would we return for HookEvents.Tag and HookEvents.Branch given the events are partially enabled (create) but not fully enabled (delete)? Was it really worth trying to abstract this data if we could not return accurate information? I decided that it was probably best to avoid this abstraction.