RFC ignore branch whitelist for pull requests

[bradrydzewski]

The yaml file has a branch filter:

pipeline: {}

branches:
  - master
  - develop

I would like to consider ignoring this filter for pull request events.

	// verify the branches can be built vs skipped
	branches, err := yaml.ParseString(conf.Data)
	if err == nil {
-		if !branches.Branches.Match(build.Branch) {
+		if !branches.Branches.Match(build.Branch) && build.Event == model.EventPush {
			c.String(200, "Branch does not match restrictions defined in yaml")
			return
		}
	}

The reason is that teams not using the github workflow (fork pull-request) will push to a branch with an open pull request, triggering two builds.

This change in logic would allow a team to whitelist branches (e.g. master) and push to feature branches with an open pull request, while only triggering a single build for the pull request, and not a second for the push. Note that merging the pull request would still trigger a build assuming it matches the branch whitelist.

I'm open to other solutions here as well ...

@jmccann @donny-dont @benschumacher hoping to get some thoughts. Does this make sense?

[benschumacher]

I'm certain that there's a detail how this change solves the issue that I'm not quite wrapping my head around, but I like the general goal of inhibiting the duplicate build. I've observed the behavior when working with repos where the workflow is feature branches vs. repo forks.

In some cases, I've introduced additional event-based, typically related to "test" or "deploy" steps:

  test:
    image: integration_tests:latest
    when:
        branch: master
        event: [ push ]

[rochdev]

We are facing this issue as well. What we would expect is for the condition to do what it actually says which is to trigger only when the actual current branch is master. The fact that it triggers on every pull requests is very counter intuitive.

[bradrydzewski]

@rochdev I am a bit unclear. Can you provide a sample of your configuration and a more detailed explanation?

[rochdev]

@bradrydzewski Our configuration is:

publish:
  when:
    branch: master

We expected this to only run when the change is merged into master, but it was running in pull requests as well until we added an event: [push] condition. This should be the default since it matches the expectation of such feature (I believe this is your proposition with this issue, correct me if I'm wrong).

In summary, I would expect branch: master to only run when the commit actually reaches the master branch.

I think maybe the current behaviour could also be kept by adding something like target: master which would build any PR pointing to master.

(For reference, we use a feature branch + master flow)

[bradrydzewski]

This proposal describes a behavior change to the top-level branches section, which is used to limit overall pipeline execution by branch [1], as opposed to per-step execution. I think you want to subscribe to #2222 instead.

In summary, I would expect branch: master to only run when the commit actually reaches the master branch.

Drone uses the target branch by design. Whether or not you think this is correct probably depends on your workflow. If you are using the github workflow (for which drone is optimized) you are forking and using feature branches. In this case, the existing implementation is optimal.

Note that we previously used the source branch (in 0.4) and eventually changed to target branch based on community feedback. So if we changed this back (as suggested) it might work better for some and then worse for others.

I think maybe the current behaviour could also be kept by adding something like target: master which would build any PR pointing to master.

I am open to tracking both source and target branch as described in #2222 and making these available as conditions. The branch field will continue to use the target branch for pull requests, since we want to avoid breaking changes of this magnitude unless absolutely necessary.

[1] http:https://docs.drone.io/hooks/#skip-branches

[ghost]

+1 on exposing origin + target branches as condition.

Maybe we even should have basic shell command conditions. To make more complicated conditions possible. Like evaluate pull request changed file extensions. And skip build if only txt, md etc.

I would think about a service docker plugin that creates conditions that can be used in steps.

[bradrydzewski]

moved to #2222