Skip to content

[JENKINS-36283] custom pr behavior#25

Closed
pascallap wants to merge 5 commits intojenkinsci:masterfrom
Nuglif:JENKINS-36283_Custom_PR_behavior
Closed

[JENKINS-36283] custom pr behavior#25
pascallap wants to merge 5 commits intojenkinsci:masterfrom
Nuglif:JENKINS-36283_Custom_PR_behavior

Conversation

@pascallap
Copy link
Contributor

@pascallap pascallap commented Nov 11, 2016

Put in place the bases to solve: Jenkins-36283
Option to choose:

  • Build origin branches (HEAD)
  • Build origin branches also filed as PRs (HEAD)
  • Build origin/fork PRs (unmerged head) (HEAD)
  • Build fork PRs (unmerged head)
  • Build origin PRs (merged with base branch)
  • Build fork PRs (merged with base branch)

@villelahdenvuo
Copy link

This is great, we're currently building all commits twice because all our prs come from local branches.

@tknerr
Copy link

tknerr commented Nov 22, 2016

+1 for this. I guess this is meant to supersede #9 then @amuniz ?

@Eernie
Copy link
Contributor

Eernie commented Nov 22, 2016

Yes I'd rather see this merged then #9 or #25 imho. Just because it has multiple options and is more like Github-source-plugin-#60.

@amuniz amuniz changed the title [Jenkins 36283] custom pr behavior [JENKINS-36283] custom pr behavior Nov 23, 2016
@amuniz
Copy link
Member

amuniz commented Nov 23, 2016

Thanks for the PR, let's wait until #23 is merged to start reviewing this, as it is changing the SCMHead representations and will have impact in your implementation.

I'll probably ping you for minor fixes. If some hard conflict happens after #23 I can take care of it.

@pascallap
Copy link
Contributor Author

Pull Request is know rebase from #23. So conflict should be already resolved.

@tknerr
Copy link

tknerr commented Jan 10, 2017

@amuniz any progress on this pull request?

looks like this would be the one that solves a lot of issues (like #26 and #9)

@abubeck
Copy link

abubeck commented Jan 11, 2017

@amuniz: A merge of this PR would also be very important for us. Let us know if we can contribute for getting this merged.

@jpigree
Copy link

jpigree commented Jan 12, 2017

@amuniz. Same here. This feature will be awesome for us.

Feel free to ask us for help aswell.

@amuniz
Copy link
Member

amuniz commented Jan 23, 2017

I'll try to review this as soon as possible, it is next in my queue.

revision = getRevision(head, hash, pr);
observer.observe(head, revision);
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion if you'd like to reduce the amount of duplication here:

bool buildPRMerge = fork ? buildForkPRMerge : buildOriginPRMerge;
bool buildPRHead = fork ? buildForkPRHead : buildOriginPRHead;
bool addHeadSuffix = buildPRMerge && buildPRHead;

if (buildPRMerge) {
    head = new PullRequestSCMHead(owner, repositoryName, branchName, pr, true);
    revision = getRevision(head, hash, pr);
    observer.observe(head, revision);
}

if (buildPRHead) {
    head = new PullRequestSCMHead(owner, repositoryName, branchName, pr, false, addHeadSuffix);
    revision = getRevision(head, hash, pr);
    observer.observe(head, revision);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I knew that there was a better way to handle this.

@jeffweiss
Copy link

Review ping. This will be a non-trivial improvement in my life. Would love to see it released.

@raulgd
Copy link

raulgd commented Jan 28, 2017

Being able to only build PRs and not branches and PRs at the same time is a big must for us as well, I'd love to get this installed on our Jenkins as soon as possible :)

@tknerr
Copy link

tknerr commented Feb 1, 2017

If there's anything we can help in testing, please let us know.

I was about to build this PR but then noticed that some other SCM related plugins are missing / not published yet in the 2.x release line (see blog post here)

Does it make sense to take this for a spin now, or better wait for a re-release of the 2.x release line of the SCM plugins?

@pascallap
Copy link
Contributor Author

This Pull Request is using the added objects from 2.x scm api.

If you want to test/evaluate functionality, you will have to use the experimental plugins repository. So you will get access to the 2.x plugin, until they are back in the stable plugin repository. (Don't do it on production system)

For the re-release of the 2.x plugins and the value to review this at the moment, @amuniz and @stephenc are probably the best persons able to answer those questions.

@luispabon
Copy link

It would be great to allow "only PRs" as well as add specific branches. This would allow us to only build PRs as well as the master branch when things are merged into it.

@abishek3876
Copy link

@amuniz , any ETA possible for this? Looks like this is a must have for many (including me).

@stephenc
Copy link
Member

stephenc commented Feb 8, 2017

Personally I was to fix the "delete everything if there is an api error" bug first and get that released before considering and additional changes

@stephenc
Copy link
Member

This really feels like replication of the death by 1000 check-boxes anti-pattern in the GitHub Branch Source. There will likely be other tweaks that will be needed on the PR building behaviour, so I would rather get the behaviour plugable and fix both plugins UI at the same time

@estyrke
Copy link

estyrke commented Feb 21, 2017

@stephenc That would be awesome, but what time frame are we talking then? Me and others have been longing for this functionality for half a year now, and at least for my own part I'd love to have death by checkboxes instead of nothing at all while we wait for a superior alternative.

@jeffweiss
Copy link

@stephenc this implementation might be death by a 1000 cuts for the handful of plugin developers working in both GU and BB, but the larger number of users of this plugin are already suffering from death by 1000 cuts of duplicate builds for origin PRs.

@tknerr
Copy link

tknerr commented Mar 15, 2017

+1 been quiet for a while but still eagerly waiting for that! 👍

@stephenc
Copy link
Member

I am against complicating the code further until I tidy up the configuration options.

@jeffweiss
Copy link

@stephenc that's incongruent with the priorities you indicated 3 weeks ago.

@kevcodez
Copy link

Any ETA for merge @stephenc ?

@fonsecas72
Copy link
Contributor

fonsecas72 commented Mar 24, 2017

Awesome job guys!
I've solved the conflicts here https://github.com/fonsecas72/bitbucket-branch-source-plugin/tree/pullrequest_with_shits as a workaround and everything seems fine (some config labels are not working but it should be some mistake I made).
I hope that you can merge this soon.

@kevcodez
Copy link

@fonsecas72 Is there a workaround to skip building PRs for now?

This pull request is about 5 months old and still not merged unfortunately.

@luispabon
Copy link

@kevcodez we have some Jenkinsfile trickery to build either master or PR branches only:

def isPr = {
    String branch = env.BRANCH_NAME;
    return branch.startsWith('PR-')
}

Then simply

node {
    if (env.BRANCH_NAME == 'master' || isPr() == true) {
        // your stuff
    }

@estyrke
Copy link

estyrke commented Mar 29, 2017

@kevcodez @luispabon We have the following in "Branch names to build automatically" in the organization folder configuration, which accomplishes the same thing: (master)|(PR-\d+). It should be possible to construct a regex that will select everything except PRs as well (maybe ^(?!PR-\d+$)?)

@kevcodez
Copy link

kevcodez commented Mar 29, 2017

@estyrke I've tried ^(?!PR-\d+$) and ^(?!PR.+)

Looking up TSU/eos-ts-core for branches
Checking branch feature/TZ-1262 from TSU/eos-ts-core
‘Jenkinsfile’ found
Met criteria
Looking up TSU/eos-ts-core for pull requests
Checking PR from TSU/eos-ts-core and branch bugfix/TZ-1193
‘Jenkinsfile’ found
Met criteria

It does not exclude PRs :(.

@fonsecas72
Copy link
Contributor

fonsecas72 commented Mar 29, 2017

@kevcodez It should work. Go and try the branch I've mentioned. You should then have checkboxes (similar to github) which should allow you to build whatever you want.

image

(ignore the label issues)

@tknerr
Copy link

tknerr commented Apr 5, 2017

@stephenc any update / approximate ETA on your earlier comment regarding the cleanup?

@nickbroon
Copy link

👍

@sasah
Copy link

sasah commented Apr 17, 2017

+1!!!

@jetersen
Copy link
Member

It is being worked on through this EPIC: https://issues.jenkins-ci.org/browse/JENKINS-43426

@stephenc
Copy link
Member

Here is a preview of the cleanup in the GitHub Branch Source. I am currently finalizing that clean-up and then I will be refactoring to make this easy to share commonality between the two plugins:

jenkins-43507

There is progress and hopefully this will explain why I have been pushing back on the checkboxes

@ghost
Copy link

ghost commented May 3, 2017

Nice preview, the option "Only branches that are not also filed as PRs" would work excellent for our use case! We are standing by to upgrade asap :)

@szhem
Copy link

szhem commented May 11, 2017

Looking forward for merge for about a year :)

@tknerr
Copy link

tknerr commented May 24, 2017

@stephenc status ping, anything ready enough we could take for a test ride?

@stephenc
Copy link
Member

@tknerr getting close... sometime next week I hope

@tknerr
Copy link

tknerr commented May 25, 2017 via email

@palfrey
Copy link

palfrey commented Jun 2, 2017

I'm trying to use @fonsecas72's version of this (as commented further up) and I'm hitting some problems:

ERROR: [Fri Jun 02 11:39:45 BST 2017] Could not fetch branches from source 28422453-5718-445e-8fe2-2b89e20326b2
java.lang.NullPointerException
	at com.cloudbees.jenkins.plugins.bitbucket.PullRequestSCMRevision.equals(PullRequestSCMRevision.java:44)
	at jenkins.branch.MultiBranchProject$SCMHeadObserverImpl.observe(MultiBranchProject.java:1918)
	at com.cloudbees.jenkins.plugins.bitbucket.BitbucketSCMSource.observeFactory(BitbucketSCMSource.java:531)
	at com.cloudbees.jenkins.plugins.bitbucket.BitbucketSCMSource.observe(BitbucketSCMSource.java:511)
	at com.cloudbees.jenkins.plugins.bitbucket.BitbucketSCMSource.retrievePullRequests(BitbucketSCMSource.java:452)
	at com.cloudbees.jenkins.plugins.bitbucket.BitbucketSCMSource.retrieve(BitbucketSCMSource.java:396)
	at jenkins.scm.api.SCMSource._retrieve(SCMSource.java:300)
	at jenkins.scm.api.SCMSource.fetch(SCMSource.java:210)
	at jenkins.branch.MultiBranchProject.computeChildren(MultiBranchProject.java:634)
	at com.cloudbees.hudson.plugins.folder.computed.ComputedFolder.updateChildren(ComputedFolder.java:219)
	at com.cloudbees.hudson.plugins.folder.computed.FolderComputation.run(FolderComputation.java:141)
	at jenkins.branch.MultiBranchProject$BranchIndexing.run(MultiBranchProject.java:973)
	at hudson.model.ResourceController.execute(ResourceController.java:98)
	at hudson.model.Executor.run(Executor.java:410)

Also, we'd like to build only PRs and the master branch. Is there any way to do this? So far, I can manage to get it to build PRs or build master, but no option for "only PRs plus regex-matched branches"

@DoDoENT
Copy link

DoDoENT commented Jun 2, 2017

I have the same use-case as @palfrey. Currently I have that implemented as custom groovy script which simply does not run the build actions if not on master branch or on PR.

@batesyuk
Copy link

batesyuk commented Jun 3, 2017

Same for me; master and beta should build and deploy on every change, other than that it should ignore all other branches and unit test PRs

@stephenc
Copy link
Member

stephenc commented Jun 3, 2017

FYI my refactoring is nearly ready. Will add merge commit builds for PRs, ability to not build origin branches that are files as a PR, lots of control over git extensions, etc... may even change the baby's nappy.

I will be looking for beta testers next week some time

@palfrey
Copy link

palfrey commented Jun 6, 2017

I started doing some work on this and if this is of any use, https://github.com/lshift/bitbucket-branch-source-plugin/tree/jenkins-36283 has

  • Fixes to the exception I saw earlier
  • Support for PRs + regex-matched branch builds
  • Sets a dummy username/email for Jenkins in Git before trying to locally merge branches so that type of build works again
  • Master merged in

@stephenc
Copy link
Member

stephenc commented Jun 6, 2017

Traits makes most of that irrelevant. The git behaviours can be attached directly. More advanced "custom" behaviours can be encapsulated in their own plugins so that only those that need the functionality clutter their UI... and only when using it

@stephenc
Copy link
Member

superseded by #53

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.