dancryer / PHPCI

PHPCI is a free and open source continuous integration tool specifically designed for PHP.
BSD 2-Clause "Simplified" License
2.42k stars 439 forks source link

Bitbucket integration creates too many builds #855

Open rainbow-alex opened 9 years ago

rainbow-alex commented 9 years ago

If I make several commits and push a build is created for every commit. This needlessly builds the project, since pushing commits A and B will build B twice anyway, right?

cooperaj commented 9 years ago

I think that in some cases you'll want the multiple builds - think about a case where a team makes commits to separate branches or parts of an application at the same time. All the commits will need testing as they're separate items. Having encountered this myself though I do agree that some thought needs to go into it as it is quite frustrating to have 10 or 15 commits queues up and all you care about is the last one.

How do other tools such as Travis or Bamboo handle it? As far as I'm aware they both do a build for every commit.

mikebronner commented 9 years ago

I would recommend the latest commit in each push. Since you have to push separately for different branches, that would still allow multiple branches to be committed to simultaneously, and yet only run one build per branch. This is how cadetship works when connected to GitHub.

cooperaj commented 9 years ago

Is it possible to track pushes as opposed to commits? Though the OP actually mentions bitbucket integration. How many api calls does bitbucket make to PHPCI? One per push or per commit. If is the former then this actually isn't too tricky.

mikebronner commented 9 years ago

It should be just a matter of setting up a POST web hook on the repo pointing to PHPCI. As per the API docs: https://confluence.atlassian.com/display/BITBUCKET/POST+hook+management

cooperaj commented 9 years ago

Yes. Thats in place already I'm guessing by the OPs comment. The issue is that bitbucket sends the json in the link you gave, including all the commits and that PHPCI has the code

foreach ($payload['commits'] as $commit) {
    ...
    $this->createBuild($project, $commit['raw_node'], $commit['branch'], $email, $commit['message']);
   ...
}

In WebhookController. i.e. it creates a build for every commit in the hook json. I'd say the decision needs to be made if this is desired behaviour and altered accordingly.

My vote is on no. This isn't desired. A single webhook call should equal a single build - regardless of number of commits contained.

mikebronner commented 9 years ago

Agreed. However, some people may want this. Perhaps a project-configuration flag is in order, and have the default be to only build once per web hook post, but allow to build all commits separately if needed. I think that would satisfy all sides?

cooperaj commented 9 years ago

It would meet my requirements yes. +1

I'm not sure if it would be possible based on my knowledge of PHPCI but if a build isn't created for a commit is PHPCI then not aware of that commit in the future? I see a case where the build hook builds the latest commit but at some point in the future a person may want to build a specific commit hash that hasn't been built in the past as it was skipped.

mikebronner commented 9 years ago

Good call, @cooperaj

Proposed Requirements:

rainbow-alex commented 9 years ago

Thanks for the quick replies guys. Latest commit in each push would solve our problem. You could take it a bit further though:

This would also handle subsequent pushes, or races between delayed hooks and impatient devs manually creating builds.

Though maybe it's not desirable to skip manual builds, ever; in that case change the behavior to "Skip pending automatic builds whenever a new build is created on the same branch.".

mikebronner commented 9 years ago

@rainbow-alex good thinking!

Perhaps this could be two different settings independent of each other?

Existing projects should not be changed (they should retain current functionality, owners can adjust settings manually), all new projects will use default values.

rainbow-alex commented 9 years ago

@mikebronner Doesn't the first option cover the second? E.g. if I push commits A and B, a build is created A, a build is created for B, and the build for B cancels the build for A. The way I interpret it it should be a set of radiobuttons or a dropdown with a third option "Never skip builds" (default).

cooperaj commented 9 years ago

@rainbow-alex They're not quite the same no. I for example would not want to skip pending builds but I would want to skip multiple builds in a single push. If developer A pushes and then B does I want two sets of tests run.

rainbow-alex commented 9 years ago

@cooperaj Doesn't phpci check out the latest commit in the branch anyway?

cooperaj commented 9 years ago

No. It checks out the commit referenced by the hash. Which is why you have your issue. Bitbucket pushes 10 commits and it builds 10 commits. Not 10 of the latest.

rainbow-alex commented 9 years ago

I've been cancelling them manually so I haven't seen which one it builds.

So in that case, I would personally still want it to build at least once per push.

rainbow-alex commented 9 years ago

But still, if you check the first checkbox in @mikebronner's two-setting example, the second one becomes moot right?

mikebronner commented 9 years ago

@rainbow-alex I see what you're saying. I haven't used it with BitBucket enough to see exactly how it sets up the pending builds. I will play with it this weekend.

Yes, if it schedules them all out in the proper order, then checking the first one would imply the second one automatically. However, checking the 2nd one would not imply the first.

And as @cooperaj says, he may want to build once per PUSH regardless of pending builds (which I agree with).

Perhaps the wording was too precise:

  1. Skip pending builds in favor of the latest. Checking only this box would only build that most recent build, regardless of number of pushes. So if 1 push came in, it would start building the latest commit in the push, but then if 3 additional pushes came in while it was building, it would skip two pushes entirely, then build the latest commit of the 3rd push.
  2. Build once per Push. Checking this box will result in builds being ran once per push, regardless if the other option is selected.

So I guess this calls into question if there ever is a situation where we would not want to build per push if there are multiple pushes?

dancryer commented 9 years ago

It's worth noting that both bitbucket and github send one webhook request per commit, not per push. We'd need to build in some logic to ignore them, but this wouldn't work if you're using the daemon as it would usually have started the first build before a second one is even created.

The situation is different for pull requests, and I agree that we should change behaviour there to just do the latest commit.

cooperaj commented 9 years ago

@dancryer ah. well that kinda makes things a little more tricky. The json example given in the docs does indicate that multiple commits can be included.

tvbeek commented 9 years ago

I think this one is in basic a copy of #656 It can be good to have an option to do only the last commit of a push. But the default for the most (or maybe all) ci tools is to test every commit. This shows you where it went wrong (in case of errors and warnings)