gitness
gitness copied to clipboard
Support additional pull request actions (closed, merged, etc)
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
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.
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
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:
- github implementation [DONE]
- database changes [DONE]
- yaml changes
- strategy for not breaking existing projects
cc @therynamo
Are there plans to implement this @bradrydzewski?
@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.
@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.
What's the status of this?
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.
I'm interested in triggering builds based on pull requests being merged--any update with this?
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.
Related: https://github.com/drone/drone/issues/2685
I gave "close" a go here:
https://github.com/drone/drone/pull/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)
@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.
@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
create
and delete
are branch actions.
open
, close
and sync
are pull request actions.
the close
action is parsed but is currently ignored.
@bradrydzewski Does this mean that a branch deletion event is now a supported trigger?
To summarize the current state of affairs:
-
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
-
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.
- Case 1:
trigger: event: - pull_request # they only expect this to be run on `opened` and `synchronized`
- 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 https://github.com/harness/drone/issues/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
andsynchronized
- 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 (https://github.com/harness/drone/pull/2902#issuecomment-581600823), 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:
- unversioned pipeline specs
- backwards compatibility
- new features
My suggestion is to unblock users is to:
- offer the interpolation of all available hooks in the user's pipeline under a feature flag
- 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
- accept that users will have to update their
exclude
syntax - 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.
- Only trigger existing actions
opened
andsynchronized
trigger: event: - pull_request
- Only trigger the inverse of the current subset; if
exclude
is used, also excludeclosed
trigger: event: - pull_request action: exclude: - synchronized
- 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.
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
Also need this to realize an end-to-end GitOps implementation.