Skip to content

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 text this 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. image

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?

What are the relevant issue numbers?

https://gitlab.com/gitlab-org/gitlab-ce/issues/20338

Edited by Dmytro Zaporozhets (DZ)

Merge request reports