gitness icon indicating copy to clipboard operation
gitness copied to clipboard

Implement pull request close events

Open andrewhowdencom opened this issue 6 years ago • 17 comments

There is currently a feature request associated with being able to run some sort of pipeline at the termination of some git event; specifically in this case a "closure" of a pull request.

This allows the consumers of drone to use drone to spin up ephemeral environments for the purpose of testing, and then later have them torn down automatically by Drone once the pull request has been closed.

This commit implements such a solution after reading the guidance on:

#2884

The commits describe the design path in further detail.

andrewhowdencom avatar Jan 14 '20 21:01 andrewhowdencom

This worked :D I've patched our drone server with it for now to see if it breaks things

andrewhowdencom avatar Jan 20 '20 19:01 andrewhowdencom

  trigger:
    event:
      include:
        - pull_request
    action:
      include:
        - closed

Which worked with

  • Creating the PR
  • Updating the PR
  • Closing the PR
  • Merging the PR

I would expect the above yaml to only work with pull request closed actions, and to ignore sync and create actions. If you want to trigger on create, sync and closed events you need to enumerate all action types.

trigger:
  event:
  - pull_request
  action:
  - closed
  - open
  - sync

I was unsure from the comment exactly how this behaved, but just wanted to make sure we were on the same page.

bradrydzewski avatar Jan 20 '20 20:01 bradrydzewski

I would expect the above yaml to only work with pull request closed actions, and to ignore sync and create actions. If you want to trigger on create, sync and closed events you need to enumerate all action types.

Sorry yes, I should have made that clear -- that's how it works presently. Rather, the initial comment:

Creating the PR Updating the PR Closing the PR Merging the PR

Meant that those were the behaviors that I tested across both pipelines in that yaml.

andrewhowdencom avatar Jan 20 '20 20:01 andrewhowdencom

There's a regression in this patch -- it somehow blocks pipelines that only include:

trigger:
  ref:
    include:
      - refs/tags/**
      - refs/heads/development

I do not know why yet; I dropped the commit off our server.

andrewhowdencom avatar Jan 20 '20 20:01 andrewhowdencom

(Edited as it was large) Testing was successfully completed based on: https://gist.github.com/andrewhowdencom/4da6514c2d01ada0244b2aca9949dd17

andrewhowdencom avatar Feb 03 '20 20:02 andrewhowdencom

Pull request is also now up to date with revised solution (See a6effe3). Will leave patched on our server for now to see how it goes, but beyond that it should work fine.

andrewhowdencom avatar Feb 03 '20 20:02 andrewhowdencom

thanks for providing the examples. the logic needs to be slightly adjusted to avoid breaking the current excludes behavior. Basically if the action is closed, the pipeline should only execute when the following evaluates to true:

if len(when.actions.includes) > 0 && when.actions.Includes("closed")

finally, we should add some unit tests to confirm the above logic: https://github.com/drone/drone/blob/master/trigger/skip_test.go

bradrydzewski avatar Feb 03 '20 20:02 bradrydzewski

Re. Unit tests I agree but also have no time at the minute;

Re. Logic I'm not sure I 100% understand. Do you mean, given a pipeline that has:

trigger:
  event:
    - pull_request
  action:
    exclude:
      - opened

It should not listen for the "closed" event? That is, practically it would mean:

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

(Both of these changes are fine but will need to wait for another day)

andrewhowdencom avatar Feb 03 '20 20:02 andrewhowdencom

the below example should not create a pipeline for a pipeline closed event. There may be teams already using the below syntax (for synchronization actions) and they will not expect this syntax to process a pull request closed event. We need to ensure this change does not break existing configurations.

trigger:
  event:
    - pull_request
  action:
    exclude:
      - opened

bradrydzewski avatar Feb 03 '20 20:02 bradrydzewski

Sounds good! That makes sense. I will tackle that as soon as I can.

andrewhowdencom avatar Feb 03 '20 20:02 andrewhowdencom

I had another go, but haven't tested it by actually releasing it yet. I wrote some tests for it based on the yaml I had before.

For now, I need sleep :sleeping: but will check this in a bit.

Edit: I Feel like there's a bug with excludes there. I can't see it though -- too tired I guess.

It's here somewhere:

// If there are still conditions but they do not match this action, the pipeline should be skipped. return true

I.e. implicit allow by lack of exclude. If you only exclude synchronize, then opened should still work.

More edit:

Found it. Probably a way to rework that logic, but that's another day's problem.

andrewhowdencom avatar Feb 03 '20 22:02 andrewhowdencom

status update: This sat on the build server for a week and didn't break anything!

andrewhowdencom avatar Feb 10 '20 16:02 andrewhowdencom

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Mar 26 '20 09:03 CLAassistant

This would be a very helpful feature to have!

natemcdowel avatar Mar 30 '20 21:03 natemcdowel

We desperately need this.

amq avatar May 18 '20 09:05 amq

The conflicts on this are non-trivial iirc, and I have limited time to resolve them. When we next upgrade our build server we'd look at it, but there's no massive reason to do that at the minute so it might be a while.

If someone wants to carry this, please feel free ♥

andrewhowdencom avatar Jun 01 '20 15:06 andrewhowdencom

Revised; not really tested yet though. Someone should consider carrying this. For now, I deployed it on an internal CI server.

andrewhowdencom avatar Jan 27 '21 18:01 andrewhowdencom