Support additional pull request actions (closed, merged, etc)

[tonglil]

As discussed on Gitter.

"Figure out all possible events drone might want to handle that it doesn't handle today"

Use case

When a PR is opened, deploy the app somewhere, and when the pr is closed, tear it down.

Related code

for github the parse code is at https://github.com/drone/drone/blob/master/remote/github/parse.go#L77:L92

right now we only have a pull_request event, so we would need a pull_request_closed event type, which would get added to https://github.com/drone/drone/blob/master/model/const.go#L3

• New database field for the flag
• Ability to toggle on in the UI

[bradrydzewski]

I think this definitely makes sense.

I'm interested in other event types that we might want to add to ensure that we don't need to make any additional design changes or abstractions. For example, if we want to support ALL tags we could consider adding an action field (this would make this a very large change, though).

Here is a list of all Bitbucket pull request actions:

Created
Updated
Approved
Approval removed
Merged
Declined
Comment created
Comment updated
Comment deleted

Here are GitHub pull request actions:

assigned
unassigned
labeled
unlabeled
opened
edited
closed
reopened

Here are some other GitHub events that we don't handle today:

create (tag or branch created)
delete (tag or branch deleted)
release

It is also important to note that drone assumes it can clone a pull request (or tag) as part of the pipeline process. So drone probably can't handle a tag deleted event because it will have nothing to clone, and won't be able to fetch the .drone.yml for the tag ref (since it was just deleted). NOTE this might also be the case for pull requests being closed, so we will need to verify.

[bradrydzewski]

there was a thread in the discourse forum:
http://discourse.drone.io/t/support-for-github-event-closed-on-issues/372/6

I will copy my comments here

[bradrydzewski]

Copied from discourse thread.
http://discourse.drone.io/t/support-for-github-event-closed-on-issues/372/6

Database Changes [COMPLETED]

First step will be to decide how we want to represent and store this data in our Go code and in our database. We will need to store the action type, as you pointed out above.

create table builds (
+build_action TEXT
);

I would suggest that action values for pull requests should be the following:

enum {
  open,
  merged,
  declined,
  synchronized,
}

The GitHub closed type would be converted to either merged or declined. I think in this case it is better to adopt the Bitbucket semantics so that we can store this information in a single field, as opposed to the GitHub approach which uses two fields (action string, merged bool).

Yaml Changes

I like the yaml syntax proposed above. I would probably slightly alter to support the below variations. Note that we have many of the required structures in place that we should be able to re-use, so supporting flexible configurations should not be am implementation issue.

when:
  event: [ pull_request ]

when:
  event:
    pull_request: merged

when:
  event:
    pull_request: [ merged, declined ]

when:
  event:
    pull_request:
      includes: merged
      excludes: declined

when:
  event:
    pull_request:
      includes: [ merged, synchronized ]
      excludes: declined

OR perhaps we use the actions section and if the action is closed or merged or re-opened (or whatever) it must be specified in the actions section?

trigger:
  event:
  - pull_request
  actions:
  - merged
  - synchronized

Avoid Breaking Changes [NEED MORE INFO]

If we started automatically triggering builds for all pull request actions it would probably cause a problem for existing configurations. They will not be expecting builds for merged or declined events, and will not have their yaml files configured with the appropriate when conditions to prevent this.

So to avoid breaking existing pipelines, this should probably be disabled by default. This will require some addition technical design. I'm not sure the best approach at this time ...

MVP Implementation

I think an initial implementation would need to include the following:

1. github implementation [DONE]
2. database changes [DONE]
3. yaml changes
4. strategy for not breaking existing projects

[bradrydzewski]

cc @therynamo

[rarneson]

Are there plans to implement this @bradrydzewski?

[ttback]

@bradrydzewski +1 My team is also interested in whether Drone can handle pull_request_updated, pull_request_closed, and pull_request_reopened properly as part of when block since we are using Drone to rigorously test new code in a PR before merging.

[rverma-nikiai]

@bradrydzewski +1 We are trying to implement a gitops and this is required to know whether its time to create a new environment or terminate it (based on event pull request open and pull request merged/declined).
Would be handy if we can also get source and target branch also.
We are currently using gitlab.

[Skeen]

What's the status of this?

[dance-cmdr]

Any update with this? We are trying too to create ephemeral environments but if we don't know when to terminate them it gets a little tricky.

[c-hanson]

I'm interested in triggering builds based on pull requests being merged--any update with this?

[bradrydzewski]

Sorry no updates. When this issue is scheduled for a future release, it will be attached to a milestone, and when work is started we will post a note in the comments.

[amq]

Related: #2685

[andrewhowdencom]

I gave "close" a go here:

#2902

It's not tested, but if the design is correct I would have the opportunity to do the due diligence in the coming week(s)

[mmccord-mdbuyline]

@bradrydzewski I'm a little confused based on what I've read in issues and code about what's been implemented with this and what's not. It seems to me that you all have implemented the following:

  open,
  merged,
  declined,
  synchronized

And based on your statements from above the plan is that merged or declined will be converted from the github closed action type, but that this work for closed has not been completed yet although the other action types have been completed. Is this true?

There's no documentation for this yet, so I'm a little in the dark.

[mmccord-mdbuyline]

@bradrydzewski -- actually I just found this too:

const (
	ActionOpen   = "open"
	ActionClose  = "close"
	ActionCreate = "create"
	ActionDelete = "delete"
	ActionSync   = "sync"
)

in hook.go so now I'm even more confused about what's been implemented and what hasn't...haha

[bradrydzewski]

create and delete are branch actions.
open, close and sync are pull request actions.

the close action is parsed but is currently ignored.

[eboddington]

@bradrydzewski Does this mean that a branch deletion event is now a supported trigger?

[tonglil]

To summarize the current state of affairs:

1. A set of actions are implemented and parsed, however they are only limited to doing very specific things internally to Drone, and are not exposed for triggering users' pipelines: https://github.com/harness/drone/blob/82a7b113edae15670efc0660ea2a7bca82cb20ad/handler/web/hook.go#L114-L125

2. The blocker at the moment for exposing these hooks to the end user's pipeline is because it will trigger unexpected builds for users who defined their pipelines using specific syntax that doesn't anticipate these actions.

   1. Case 1:

   trigger:
     event:
     - pull_request
   # they only expect this to be run on `opened` and `synchronized`

   1. Case 2:

   trigger:
     event:
     - pull_request
     action:
       exclude:
       - synchronized
   # they only expect this to be run on `opened`

---

As for a "strategy for not breaking existing projects"...

Avoid Breaking Changes [NEED MORE INFO]

If we started automatically triggering builds for all pull request actions it would probably cause a problem for existing configurations. They will not be expecting builds for merged or declined events, and will not have their yaml files configured with the appropriate when conditions to prevent this.

So to avoid breaking existing pipelines, this should probably be disabled by default. This will require some addition technical design. I'm not sure the best approach at this time ...

We do not want to automatically enable these actions by default. However, there is an existing issue where we are discussing ways we can let people opt-in to additional event types. Please subscribe to #1878 for updates.

This is my proposal:

• All triggers that specify the pull_request condition that do not have an action specified get interpolated by the engine with the default actions: opened and synchronized
• All steps that specify the branch condition that do not have an action specified get interpolated by the engine with the default action: create

In situations like Case 2 (#2902 (comment)), I don't think there's a way to provide a "compatible" upgrade as the use of the exclude syntax forced the definition into a corner - using exclude in the specification will not scale to future additions of this or any other field.

At some point, the Drone project will need to realize it can only pick 2 of the three:

1. unversioned pipeline specs
2. backwards compatibility
3. new features

My suggestion is to unblock users is to:

1. offer the interpolation of all available hooks in the user's pipeline under a feature flag
2. discourage the use of exclude syntax for fields that can be expanded upon in the future, and be explicit about the options they want to use:

   trigger:
     event:
     - pull_request
     action:
       - opened

3. accept that users will have to update their exclude syntax
4. switch the opt-in feature flag into an opt-out feature flag to allow installations time and flexibility to migrate their pipelines to not use exclude

---

If that is not acceptable, then an alternative proposal that maintains compatibility temporarily (while the fields exist as they do today) but makes the end-user experience more painful: only trigger the new actions when they are explicitly written out.

1. Only trigger existing actions opened and synchronized

   trigger:
     event:
     - pull_request

2. Only trigger the inverse of the current subset; if exclude is used, also exclude closed

   trigger:
     event:
     - pull_request
     action:
       exclude:
       - synchronized

3. Only trigger closed if it is explicitly listed.

   trigger:
     event:
     - pull_request
     action:
       - opened
       - synchronized
       - closed

---

I sincerely and strongly hope Drone is able to move forward with this feature as it's something that has been waited for on a long time and would be a huge improvement that is worthwhile making changes to use it.

[tonglil]

Hoping there will be some movement on this.

If we started automatically triggering builds for all pull request actions it would probably cause a problem for existing configurations. They will not be expecting builds for merged or declined events, and will not have their yaml files configured with the appropriate when conditions to prevent this.

One thought I had was that when a PR is closed, it doesn't really affect this issue since when the PR is closed, the clone would fail as the merge ref won't exist for the branch right?

One of the main use case this issues unlocks is running post-PR close actions, such as deleting namespaces. Here is an example of what this can do in Codefresh: https://codefresh.io/docs/docs/ci-cd-guides/preview-environments/#cleaning-up-temporary-environments

[lvijnck]

Also need this to realize an end-to-end GitOps implementation.