Multibranch event listener for Github PR labels

[Steven F]

Hi,

I'm trying to write a filter for multibranch which accepts PRs with a given github PR label. That part was simple. I also thought I should implement an events subscriber (GHEventsSubscriber) and I'm having trouble figuring out the details of that.

I've tried basing it on the PullRequestGHEventSubscriber since it will be creating/deleting heads rather than triggering builds but I couldn't figure out how some things were used, specifically the heads() method. My class is pretty much a copy paste with the actions changed to labeled/unlabeled. Right now, I'm getting messages like

[Wed Mar 14 13:37:45 GMT 2018] Received Pull request #9 labeled in repository sfoster/branch_filter_test CREATED event from 10.32.8.197 ⇒ http://10.32.8.83:8080/jenkins/github-webhook/ with timestamp Wed Mar 14 13:37:40 GMT 2018

Found match against demo (new branch PR-9)

[Wed Mar 14 13:37:47 GMT 2018] Finished processing Pull request #9 labeled in repository sfoster/branch_filter_test CREATED event from 10.32.8.197 ⇒ http://10.32.8.83:8080/jenkins/github-webhook/ with timestamp Wed Mar 14 13:37:40 GMT 2018. Matched 1.

 in jenkins.branch.MultiBranchProject.log, and

[Wed Mar 14 13:37:47 GMT 2018] Pull request #9 labeled in repository sfoster/branch_filter_test CREATED event from 10.32.8.197 ⇒ http://10.32.8.83:8080/jenkins/github-webhook/ with timestamp Wed Mar 14 13:37:40 GMT 2018 processed in 2.3 sec

in the projects  "repository events" page, but no sign of the new PR in the project.

I guess I'm not even sure how a successful result would play out, does it trigger a repository indexing with the given Event? or do the new heads just appear in the project?

[Jesse Glick]

On Wed, Mar 14, 2018 at 10:50 AM, Steven F <steven...@gmail.com> wrote:
> I'm not even sure how a successful result would play out, does it trigger a repository indexing with the given Event? or do the new heads just appear in the project?

Event subscribers should be able to add new heads immediately. The
whole point of having this complicated API is to be able to bypass a
full branch indexing run, which can be expensive.

As to the rest of your question, I am not sure. You read the docs right?

[Michael Neale]

Hi Steven. 

This seems to be a reasonably often asked for feature (I wasn't able to find a ticket though, for multibranch/github multibranch specifically, surely there is one?). 

Are you thinking that ideally in your case the branches/PRs would not appear at all until the label is applied? Would an alternative that makes it show up as not built or similar do? (may be easier to do that...)

[Steven F]

On Thursday, March 15, 2018 at 1:07:31 AM UTC, Michael Neale wrote:

Hi Steven. 

This seems to be a reasonably often asked for feature (I wasn't able to find a ticket though, for multibranch/github multibranch specifically, surely there is one?). 

Are you thinking that ideally in your case the branches/PRs would not appear at all until the label is applied? Would an alternative that makes it show up as not built or similar do? (may be easier to do that...)

I didn't see a ticket either, but I think a PR label filter is mentioned as an example use case in one of the API implementation docs. Unlabeled PR jobs not appearing in Jenkins is the goal for me, and I actually have a filter written that works with polling/indexing. I could stop now and enable polling on the multibranch project but it'd be nice to keep polling to a minimum of course. Hopefully I'm just missing something simple to get the new event subscriber up and running.

On Wednesday, March 14, 2018 at 3:02:16 PM UTC, Jesse Glick wrote:

On Wed, Mar 14, 2018 at 10:50 AM, Steven F <steven...@gmail.com> wrote: 

> I'm not even sure how a successful result would play out, does it trigger a repository indexing with the given Event? or do the new heads just appear in the project? 

Event subscribers should be able to add new heads immediately. The 
whole point of having this complicated API is to be able to bypass a 
full branch indexing run, which can be expensive. 

As to the rest of your question, I am not sure. You read the docs right? 

I had overlooked the docs in scm-api and those helped me understand some of the concepts and narrow things down, thanks. I think the bit I'm not following now is more in the github branch source implementation. My SCMHeadEvent returns the PR and its branch (which is probably discarded by the set of filters), but I was unable to return a PullRequestSCMRevision for the PR head because its constructor is package private. Then, I couldn't tell if that was important because I couldn't trace where these returned heads are used in github branch source. I don't know if the resulting source fetch is scoped to these heads.

Sorry if this is all basic, my Java isn't that strong :)

[Jesse Glick]

On Thu, Mar 15, 2018 at 4:44 AM, Steven F <steven...@gmail.com> wrote:
> I was unable to return a PullRequestSCMRevision for
> the PR head because its constructor is package private. Then, I couldn't
> tell if that was important because I couldn't trace where these returned
> heads are used in github branch source. I don't know if the resulting source
> fetch is scoped to these heads.

It is important. When a PR branch project is built, `checkout scm` and
related features will select a particular commit from the PR (not
always the most recent), and may additionally merge it against a
particular commit in the base branch.

> Sorry if this is all basic, my Java isn't that strong

I am afraid the multibranch system has become a rather complex API
with the addition of events and traits. It is not easy even for
experienced Jenkins developers to follow the details.

[Steven F]

On Friday, March 16, 2018 at 3:05:30 PM UTC, Jesse Glick wrote:

It is important. When a PR branch project is built, `checkout scm` and
related features will select a particular commit from the PR (not
always the most recent), and may additionally merge it against a
particular commit in the base branch.

Ok, the reason I thought it might not be important is that GHBS's PullRequest event returns a null revision in case of a PR merge, so I was testing that as a first step. I just noticed that the PullRequestSCMHead is used by the GHBS events, but its class constructor is also package private while I've been using regular SCMHead. I still haven't found where these returned heads are later referenced, but I'm worried that GHBS will reject any non-PullRequestSCMHeads or revisions in which case there's nothing I can do here.

[Jesse Glick]

On Fri, Mar 16, 2018 at 11:21 AM, Steven F <steven...@gmail.com> wrote:
> GHBS's
> PullRequest event returns a null revision in case of a PR merge

I suppose you are referring to

https://github.com/jenkinsci/github-branch-source-plugin/blob/245300dd6c2e1667b11155e2772b30c5f435f0ca/src/main/java/org/jenkinsci/plugins/github_branch_source/PullRequestGHEventSubscriber.java#L325-L328

So far the only case I can find where any caller pays attention to the
values in the map returned by this API is

https://github.com/jenkinsci/branch-api-plugin/blob/dcc0d23a0870058c6138362d3aa017c0d56effc2/src/main/java/jenkins/branch/MultiBranchProject.java#L1549-L1569

which seems to be a performance optimization, to avoid an extra call
to `SCMSource.retrieve` to look up the current head revision.

Stephen Connolly wrote most of this code and would be best placed to
answer questions like this.

[Steven F]

I locally modified GHBS to make the constructors for PullRequestSCMHead and PullRequestSCMRevision public which has seemingly solved the problem. It is also correctly scoping the source request to the PR involved in the event. I don't know enough about GHBS to know if exposing these constructors would cause issues though. It seems like the package-private decision was made here

[Stephen Connolly]

On Mon 19 Mar 2018 at 00:50, Steven F <steven...@gmail.com> wrote:

I locally modified GHBS to make the constructors for PullRequestSCMHead and PullRequestSCMRevision public which has seemingly solved the problem.

What did I do in Gitea plugin?

If I made the constructor public there, then that’s what we should probably be doing in GitHub/ Bitbucket 

(Gitea is my “no hacks or legacy migration complexity reference implementation”)

So to be clear:

1. You have a filter trait that restricts PRs based on the presence/absence of a number of labels. With that, scanning should work. PR commit updates should work (ie if the PR meets the label criteria and someone updates the commit)

2. You want to have an event listener for PR labelled and unlabelled events. This event listener will return heads with a revision when the PR starts matching the label criteria and heads with a null revision when they stop. All other PR events can continue to be processed as before

Ideally you’d allow things like:

* only discover PRs labelled with “needs-check”

* ignore PRs labelled with “work-in-progress”

And a final question, would a BranchBuildStrategy work better (ie discover all PRs but only build one with the matching label criteria) - more asking to see what your use case is

(I think it would be really cool if you get this enabled as an extension plugin By The Way, and kudos to you)

So:

* if gitea has public constructors for PR head and revision, please file a PR against GitHub to make constructors public. Mention that this PR is “an enabling change for extension plugin”

* check that your extension plugin is only filling the event gap (ie labelled and unlabelled events) otherwise it will conflict with the other events 

Really cool feature. Glad the docs helped you. Please file PRs for any docs improvements you can think of.

It is also correctly scoping the source request to the PR involved in the event. I don't know enough about GHBS to know if exposing these constructors would cause issues though. It seems like the package-private decision was made here

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/c8805fee-82e4-4fbd-861b-586cfae41c58%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--

Sent from my phone

[Stephen Connolly]

On Mon 19 Mar 2018 at 08:13, Stephen Connolly <stephen.al...@gmail.com> wrote:

On Mon 19 Mar 2018 at 00:50, Steven F <steven...@gmail.com> wrote:

I locally modified GHBS to make the constructors for PullRequestSCMHead and PullRequestSCMRevision public which has seemingly solved the problem.

What did I do in Gitea plugin?

If I made the constructor public there, then that’s what we should probably be doing in GitHub/ Bitbucket 

(Gitea is my “no hacks or legacy migration complexity reference implementation”)

So to be clear:

1. You have a filter trait that restricts PRs based on the presence/absence of a number of labels. With that, scanning should work. PR commit updates should work (ie if the PR meets the label criteria and someone updates the commit)

2. You want to have an event listener for PR labelled and unlabelled events. This event listener will return heads with a revision when the PR starts matching the label criteria and heads with a null revision when they stop. All other PR events can continue to be processed as before

Ideally you’d allow things like:

* only discover PRs labelled with “needs-check”

* ignore PRs labelled with “work-in-progress”

And a final question, would a BranchBuildStrategy work better (ie discover all PRs but only build one with the matching label criteria) - more asking to see what your use case is

Thinking out loud:

The branch build strategy would allow all PRs to be discovered, but only build the ones with the label criteria match. As such, it could miss some of the “labelled” and “unlabelled” changes unless the branch revision has also changed since last build... need to think that through

Another approach could be to add the labels to the PullRequestSCMHead as a mixin. This would make the label info part of the equality contract, so changing labels would trigger a rebuild... but would enable categories based on PR labels... and a BranchBuildStrategy could suppress the rebuild on label change.

In any case, I see a cohort of users who do not want to have PRs that do not match the label criteria to even be displayed (even though displaying without auto-build would allow manual builds of PRs without label criteria, the users may not want the distraction) so I see a SCMFilterTrait and corresponding event listener to fill the event gap as a Good Thing™️, but we may need think about the other lines of attack for the other cohorts of users too!

(I think it would be really cool if you get this enabled as an extension plugin By The Way, and kudos to you)

So:

* if gitea has public constructors for PR head and revision, please file a PR against GitHub to make constructors public. Mention that this PR is “an enabling change for extension plugin”

* check that your extension plugin is only filling the event gap (ie labelled and unlabelled events) otherwise it will conflict with the other events 

Really cool feature. Glad the docs helped you. Please file PRs for any docs improvements you can think of.

It is also correctly scoping the source request to the PR involved in the event. I don't know enough about GHBS to know if exposing these constructors would cause issues though. It seems like the package-private decision was made here

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/c8805fee-82e4-4fbd-861b-586cfae41c58%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--

Sent from my phone

[Stephen Connolly]

On Mon 19 Mar 2018 at 08:39, Stephen Connolly <stephen.al...@gmail.com> wrote:

On Mon 19 Mar 2018 at 08:13, Stephen Connolly <stephen.al...@gmail.com> wrote:

On Mon 19 Mar 2018 at 00:50, Steven F <steven...@gmail.com> wrote:

I locally modified GHBS to make the constructors for PullRequestSCMHead and PullRequestSCMRevision public which has seemingly solved the problem.

What did I do in Gitea plugin?

If I made the constructor public there, then that’s what we should probably be doing in GitHub/ Bitbucket 

(Gitea is my “no hacks or legacy migration complexity reference implementation”)

So to be clear:

1. You have a filter trait that restricts PRs based on the presence/absence of a number of labels. With that, scanning should work. PR commit updates should work (ie if the PR meets the label criteria and someone updates the commit)

2. You want to have an event listener for PR labelled and unlabelled events. This event listener will return heads with a revision when the PR starts matching the label criteria and heads with a null revision when they stop. All other PR events can continue to be processed as before

Ideally you’d allow things like:

* only discover PRs labelled with “needs-check”

* ignore PRs labelled with “work-in-progress”

And a final question, would a BranchBuildStrategy work better (ie discover all PRs but only build one with the matching label criteria) - more asking to see what your use case is

Thinking out loud:

The branch build strategy would allow all PRs to be discovered, but only build the ones with the label criteria match. As such, it could miss some of the “labelled” and “unlabelled” changes unless the branch revision has also changed since last build... need to think that through

Ok thought experiment:

PR has label criteria already, revision updated, so because revision != last build revision => consult strategy

PR has label criteria and transitions to no label criteria, so no revision change => no need to rebuild anyway

PR does not have label criteria, revision updated and still no label criteria. Because revision != last build revision => consult strategy (which says “no build” because does not meet label criteria)

PR does not have label criteria and transitions to having label criteria. This is the case where we could have issues... now would we?

If no previous build (should be the normal case) revision != last build revision => consult strategy

If previous build (because there was a manual build, or the label was removed & added again): we may have the case where revision == last build revision, in which case, no build triggered.

So we can lose build events if either the label criteria is broken and restored without a revision change to the PR or if a manual build of the PR was triggered *before* the label criteria is established. 

Neither case seems particularly worrisome as they are “defensible”...

Though, if the user has configured the Jenkinsfile to behave differently depending on the labels, the user *may* want to trigger a rebuild on label change... but perhaps we can address that in branch-api by consulting the branch build strategies even when the revision matches the last built revision...

NOTE TO SELF: TL;DR probably do not need to add labels to PullRequestSCMHead, rather return them as an action from fetchActions... that would be much less risky for build storms

Another approach could be to add the labels to the PullRequestSCMHead as a mixin. This would make the label info part of the equality contract, so changing labels would trigger a rebuild... but would enable categories based on PR labels... and a BranchBuildStrategy could suppress the rebuild on label change.

In any case, I see a cohort of users who do not want to have PRs that do not match the label criteria to even be displayed (even though displaying without auto-build would allow manual builds of PRs without label criteria, the users may not want the distraction) so I see a SCMFilterTrait and corresponding event listener to fill the event gap as a Good Thing™️, but we may need think about the other lines of attack for the other cohorts of users too!

(I think it would be really cool if you get this enabled as an extension plugin By The Way, and kudos to you)

So:

* if gitea has public constructors for PR head and revision, please file a PR against GitHub to make constructors public. Mention that this PR is “an enabling change for extension plugin”

* check that your extension plugin is only filling the event gap (ie labelled and unlabelled events) otherwise it will conflict with the other events 

Really cool feature. Glad the docs helped you. Please file PRs for any docs improvements you can think of.

It is also correctly scoping the source request to the PR involved in the event. I don't know enough about GHBS to know if exposing these constructors would cause issues though. It seems like the package-private decision was made here

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/c8805fee-82e4-4fbd-861b-586cfae41c58%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--

Sent from my phone

--

Sent from my phone

[Steven F]

On Monday, March 19, 2018 at 8:13:59 AM UTC, Stephen Connolly wrote:

What did I do in Gitea plugin?

If I made the constructor public there, then that’s what we should probably be doing in GitHub/ Bitbucket 

In Gitea, it seems the SCMHead has a public constructor but the SCMRevision has a protected constructor. My tests have all been with the PR Merge strategy so far, so the SCMRevision hasn't come into play yet (taking the lead from the GH pull request events, it's returned as null from the SCMEvent for merges).

On Mon 19 Mar 2018 at 08:13, Stephen Connolly <stephen.al...@gmail.com> wrote:

And a final question, would a BranchBuildStrategy work better (ie discover all PRs but only build one with the matching label criteria) - more asking to see what your use case is

 

The build solution I have is a bit wacky, involving a multibranch project per build platform (10+). This is because each platform requires a level of paralellization for things like compiler versions and build configurations. It's also very useful to have platforms as a manually buildable unit. I also have a parent/root multibranch project which kicks off a branch/pr in each platform build and collects the results. This acts as the build result for inspection by other tools. All of the platform builds use the "Suppress automatic SCM triggering" property (which I thought WAS a branch build strategy until I looked it up just now, so maybe that's irrelevant). With so many multibranch projects, keeping indexing to a minimum is a great bonus which is what I am hoping to achieve with the filter + event system.

(https://issues.jenkins-ci.org/browse/JENKINS-43749?focusedCommentId=319032&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-319032 would tame this setup somewhat but that's a problem for the future)

[Steven F]

I have opened a PR to make the head and revision constructors public https://github.com/jenkinsci/github-branch-source-plugin/pull/179

I've also thought about having an EnvironmentContributor to make the labels available to a build (my use case would be as 'when' conditions for stages) but it doesn't seem like a simple task at the moment.

It sounds like this suggestion would be helpful for this, too:

On Monday, March 19, 2018 at 8:39:39 AM UTC, Stephen Connolly wrote:

Another approach could be to add the labels to the PullRequestSCMHead as a mixin.

although I'm not really sure what is meant by a mixin vs a property

[Steven F]

On Monday, March 19, 2018 at 10:12:03 AM UTC, Stephen Connolly wrote:

NOTE TO SELF: TL;DR probably do not need to add labels to PullRequestSCMHead, rather return them as an action from fetchActions... that would be much less risky for build storms

I've been giving this a stab myself but I'm a bit lost. I tried the mixin approach first but it didn't work out, I'm not sure changing labels after the SCMHead was created would have been handled correctly.

For now I'm trying to store the labels inside the ObjectMetadataAction in scm-api which is convenient for injecting an environment variable from branch-api, but that might not be the right place for it.

The real trouble has been trying to figure out how github branch source populates this. I think most cases are handled in the CacheUdatingIterable.observe method, so I added

@Override
public void observe(GHPullRequest pr) {
    int number = pr.getNumber();
    List<String> labelNames = new ArrayList<>();
    try {
        Collection<GHLabel> labels = pr.getLabels();
        for (GHLabel label : labels) {
            labelNames.add(label.getName());
        }
    } catch (IOException e) {
        //
    }

with this code, I get a null pointer exception slightly later at user.getName(), I'm not sure if this is a bug with the testing environment or what. It seems like the GitHub root is set to null at this point if I retrieve the labels.

pullRequestContributorCache.put(number, new ContributorMetadataAction(
        user.getLogin(),
        user.getName(),
        user.getEmail()
));

[Stephen Connolly]

You’d want an entirely new action for the labels... at least that’s what my 1am brain thinks

--

You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/c19634f6-3b58-40c1-9a44-23f3f9375afb%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.