Modularize dependencies for GH payload consumption and Slack message sending

[yasunariw]

Description of the task

Additions

Modularize dependencies for GH payload consumption and Slack message sending

Identified the minimal set of functions that need to be configurable during local vs remote handling, and extracted them to an interface in api.ml. Created local (api_local.ml) and remote (api_remote.ml) implementations of this interface. Turned action.ml into a functor that consumes these implementations. Reused the same functions in action.ml as much as possible in both the production and test environment (see notabot.ml and test.ml). This should extend test coverage and make it easier to triage when something goes wrong in production that worked in testing.

(The name of this branch is misleading. I initially thought the whole Context needs to be differentiated b/w remote and local (see #90), but it in fact only the HTTP calls to Slack and GH needed it.)

Improve documentation

Documented more edge case behaviors in config options.

Bug Fixes

• Remote config file was being fetched twice per payload
• Remote commit (from commit comment) was being fetched twice per payload
• Behavior described in documentation contradicted actual behavior

Other

• Prefer library functions over DIY solutions for list handling
• Avoid manually assembling GH URLs when they are available in the payload
• Authenticate all requests to GH using authorization header instead of query param (will be deprecated)
• Generally de-spaghettify the code to maintain separation of concerns

How to test

All tests should pass:

make test

References

• closes mock HTTP requests in tests #88, expose interface to streamline remote and local handling logic #90

[ygrek]

For fields that expect a list, there's now a clearer semantic distinction between the absence of the field and the presence of an empty list. If the field specifies a whitelist, the former should denote "allow everything" and the latter should denote "ignore everything".

What is the usecase for this distinction?

[ygrek]

bug fixes and improvements sound good, but they should have been done in separate PRs or on master, in current shape this PR is impossible to review at large, too many logic changes, the title "Modularize dependencies for GH payload consumption and Slack message sending" is definitely a lie. Hence the review is cursory.

[yasunariw]

For fields that expect a list, there's now a clearer semantic distinction between the absence of the field and the presence of an empty list. If the field specifies a whitelist, the former should denote "allow everything" and the latter should denote "ignore everything".

What is the usecase for this distinction?

As a user, if I'm told that allow: [a,b] means allow only a and b, and allow: [a] means allow only a, then wouldn't I expect allow: [] to mean allow nothing, and thus an undefined allow field to mean allow everything?

[ygrek]

then wouldn't I expect allow: [] to mean allow nothing, and thus an undefined allow field to mean allow everything?

yes, but what is the usecase fpr the rule that allows nothing? it will never match, so there is no usecase for it. I guess the change is alright from consistency pov, but I don't like unnecessary changes (e.g. now you need to ensure this change is applied to deployed configs as well, luckily there is only one currently)

[yasunariw]

yes, but what is the usecase fpr the rule that allows nothing? it will never match, so there is no usecase for it. I guess the change is alright from consistency pov, but I don't like unnecessary changes (e.g. now you need to ensure this change is applied to deployed configs as well, luckily there is only one currently)

That's fair. I've reverted the changes and updated the description. This PR doesn't break existing deployed configs anymore.

[ygrek]

minor comment, lets get this merged