search

softdevteam / mattermost-github-integration

GitHub integration for Mattermost
Other
79 stars 50 forks source link

Added support for HMAC signature. #3

Closed jaheba closed 8 years ago

jaheba commented 8 years ago

This pull request adds support for Github's signature feature.

ptersilie commented 8 years ago

We also need to add the SECRET = '' to the config.template

jaheba commented 8 years ago

Right. The current approach allows to not have SECRET specified without causing an error. But I'll just add it to the config and then we can remove the getattr check.

ptersilie commented 8 years ago

That's a fair point. But I think by having it in the template it's more clear that it can be used.

jaheba commented 8 years ago

Should we add a config validation? So that all keys are tested for existence?

ptersilie commented 8 years ago

That would certainly make sense at some point when the config get's bigger. I don't feel its immediately necessary at the moment though, given the config and the people using the integration is fairly small at the moment. But feel free to add this in another PR if you are bored. ;)

ptersilie commented 8 years ago

Okay, can you squash this PR, then I will merge it. Did you test if the SECRET stuff is working in practise?

jaheba commented 8 years ago

Squash?

And yes, it is (hand) tested.

ptersilie commented 8 years ago

Squashing means combining some or all of the commits in this PR into one to clean up the commit messages. For instance the "Typo" and my commit messages don't provide any useful information. For this PR I would suggest squashing all commits into a single one with the commits message being the message from the first commit you made. See more here: https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History#Squashing-Commits

jaheba commented 8 years ago

Done

ptersilie commented 8 years ago

Looks good. Merged.