softdevteam / mattermost-github-integration

GitHub integration for Mattermost
Other
79 stars 50 forks source link

Add check for ignore ref_type #45

Closed Scriptkiddi closed 6 years ago

Scriptkiddi commented 6 years ago

If an event is create or delete data['action'] does not exist so it can not be ignored thats why another check for data['ref_type'] was necessary

Scriptkiddi commented 6 years ago

Hey any chance of this getting merged

ptersilie commented 6 years ago

Of course! Sorry I didn't get around looking at this earlier. You are right, create and delete don't have an action, so thanks for the fix! However, I think the current change would cause a KeyError if we add create to the GITHUB_IGNORE_ACTIONS, since data['action'] would then be checked first. Would you mind adding a check that makes sure this doesn't happen?

Scriptkiddi commented 6 years ago

Thanks for answering so quickly, I do not see where a KeyError could be raised since the conditions are evaluated in turn so its ensured event is defined and the two checks for data['action'] or data['ref_type'] are always returning true or false But maybe Im not seeing it right now

ptersilie commented 6 years ago

What I meant is that data['action'] in config.GITHUB_IGNORE_ACTIONS[event] will raise a KeyError if the event is create/delete since those events, as you pointed out, have no 'action' key. We probably need to check which event it is and then check only data['action'] or data['ref_type'].

Scriptkiddi commented 6 years ago

@ptersilie bump

ptersilie commented 6 years ago

Merged. Thanks!