Filter web hooks by branch
What does this MR do?
Allow for filtering of project webhooks by branch as requested per https://gitlab.com/gitlab-org/gitlab-ce/issues/20338
Scope:
- ability to specify a single branch name in a form field under "Push events" when configuring a web hook
- logic to respect this branch name restriction, so that when a branch name is configured, only pushes to that branch result in a web hook being triggered
- ability to set and get this value using the API
- project webhooks only
Feature acceptance criteria
- if a branch filter is specified AND the push event is for that branch, the hook is run
- if a branch filter is specified AND the push event is NOT for that branch, the hook is NOT run
- if a branch filter is NOT specified, the hook is run for a push event regardless of branch
- if the hook is not a push event, none of this applies - hooks are run as they are now
Future:
- support comma-separated branch names
- support for branch patterns with wildcards (
*
), like we do with protected branches. - support for web hook events other than push events
TODO
-
validate format of branch name (maybe using Gitlab::GitRefValidator.validate(branch_name)), trim -
add tests for the form field -
expose in api (figure out how it affects versioning) -
understand what is the import/export safe attributes thing -
find a better name than #active_for_branches? as it is misleading as it implies the hook event must also be active -
put in some more tests and maybe better style for nil and empty string cases of push_events_branches -
squash stuff - I would leave the database change and the initial refactor as separate commits though -
[ ] i18n for form placeholder textthis page isn't translated yet -
and check all the awesome stuff in the acceptance criteria -
test it end to end with feature acceptance criteria - ~~[ ] see if we agree on where to ultimately put the filter logic
-
maybe we should call it push_events_branch_filter instead of push_events_branch_filters wah -
update documentation to mention wildcard
Are there points in the code the reviewer needs to double check?
Database change: this MR adds a column push_events_branch_filters
to the web_hooks
table to store the branches as a string. I decided not to add a column string limit because the pattern seems to be not to not add one unless it makes sense for the validation (eg for url).
I implemented the ffilter on the ProjectHook model for now.
It is a bit weird that if left blank in the UI it defaults to empty string whereas if left blank in the API create it defaults to null. But looks like other fields have that behaviour.
Why was this MR needed?
Screenshots (if relevant)
A field is added on the webhooks form in settings/integrations.
Output of migration
== 20180607071808 AddPushEventsBranchFilterToWebHooks: migrating ==============
-- add_column(:web_hooks, :push_events_branch_filter, :text)
-> 0.0005s
== 20180607071808 AddPushEventsBranchFilterToWebHooks: migrated (0.0006s) =====
Does this MR meet the acceptance criteria?
-
Changelog entry added, if necessary -
Documentation created/updated -
API support added -
Tests added for this feature/bug - Conform by the code review guidelines
-
Has been reviewed by a UX Designer -
Has been reviewed by a Frontend maintainer -
Has been reviewed by a Backend maintainer -
Has been reviewed by a Database specialist
-
-
Conform by the merge request performance guides -
Conform by the style guides -
If you have multiple commits, please combine them into a few logically organized commits by squashing them -
Internationalization required/considered -
End-to-end tests pass ( package-and-qa
manual pipeline job)
What are the relevant issue numbers?
Edited by Dmytro Zaporozhets (DZ)