Lookup previous build using build id and re-use any parameters

[acormier1]

When using the generic-webhook-plugin, webhook payloads are parsed (via jsonpath) and injected into the original build as parameters. When re-requesting a check from github, we need the ability to look up the parameters of the original build so that the build has the parameters from the original build. Otherwise empty/default parameters would cause the build to fail (even which git hash we check out is driven by a parameter).

In order to accomplish this, we use the jenkins externalized build id as the external_id when publishing the check. When the check is re-triggered, we are able to look up the original build and re-inject the parameters.

I believe this may be a generic solution to #223. Our issue is that instead of a single parameter, we have 10-20 parameters so we needed a solution which is able to re-populate those parameters.

I realize there are risks with changing the mechanism by which we look up the job. What I would like some feedback on is whether this change should be compatible with the original intent of the plugin (via a multibranch pipeline). There is also a risk that the original build has fallen off the history and can no longer be looked up, but it's unclear to me how big of an issue that would be in practice.

[timja]

Taking generic webhook plugin out of the picture here, did re-populating parameters work before or was that just missing?

[acormier1]

Taking generic webhook plugin out of the picture here, did re-populating parameters work before or was that just missing?

That was always missing. The existing implementation launches a new build from the job id stored in the external_id checks field. I'm guessing this ONLY would have worked using multi-branch? I believe this PR would extend the use-case of this plugin to a regular pipeline.

However, the reason I mentioned generic-webhook-plugin is because I don't know what other context parameters would be used when initially triggering a job from jenkins. We are migrating to github from an environment (bitbucket) where we utilized parameters extensively. Generic-webhook-plugin allows us to keep these parameters and map the webhook payload to the existing parameters we have making our migration path much simpler.

[timja]

concept seems fine, if you can fix the build and testing passes we can take it

[acormier1]

concept seems fine, if you can fix the build and testing passes we can take it

Absolutely, will do.

[acormier1]

Looking at the GitHubChecksPublisherITest.java failures I seem to have a lack of understanding of what it means to publish a check "from a job". There are quite a few conditionals in that file and specific testing of the checks coming from a job.

In my mental model, a check would ALWAYS be published from within a specific run/build of a job, not the job itself so this distinction is throwing me off a bit. Can anyone help enlighten me as to what I'm missing here?

        if (fromJob) {
            return GitHubSCMSourceChecksContext.fromJob(job, urlProvider.getJobURL(job), scmFacade);
        }
        return GitHubSCMSourceChecksContext.fromRun(run, urlProvider.getRunURL(run), scmFacade);

[timja]

I think the initial Queued check is done before the run exists and it's set to the job.

@XiongKezhi do you remember?

[acormier1]

I think the initial Queued check is done before the run exists and it's set to the job.

@XiongKezhi do you remember?

I think I see the difference between QUEUED and the other status checks here: https://github.com/jenkinsci/checks-api-plugin/blob/0a83c62e4f728c6b57018dc36b3fa23ac8850c0f/src/main/java/io/jenkins/plugins/checks/status/BuildStatusChecksPublisher.java#L143. QUEUED seems to use the fromJob() as you mentioned so thanks for that pointer.

Digging deeper it seems that this plugin has two functions to resolve the SHA used for the check, one for the job and one for the run:

github-checks-plugin/src/main/java/io/jenkins/plugins/checks/github/GitHubSCMSourceChecksContext.java

Line 114 in 1c4caf9

private String resolveHeadSha(final Run<?, ?> theRun) {

What I can't quite wrap my head around is how the function which gets the SHA from the job is guaranteed to match the SHA from the run within a Github pull request context. This is especially confusing when the Job's HEAD/SHA is dynamic based on an incoming parameters like ours. Can you give an example of where the Run/Job configuration is guaranteed to have the same SHA?

Appreciate the guidance here.

[acormier1]

Might have answered my own question by continuing to dig.

Using the github branch source plugin, each Pull Request actually shows up as it's own job: https://ci.jenkins.io/job/Plugins/job/github-checks-plugin/view/change-requests/

In this case, I can understand how both the job and the run would have the same hash since the job is configured using the PR branch.

[timja]

I don't understand your question to be honest. But hopefully you figured out what you needed

[timja]

Bad merge by the looks of it, the behaviour was changed in f35f09a

Suggested change

	@Test
	void shouldThrowExceptionWhenHeadBranchMissingFromPayload() throws IOException {
	assertThatThrownBy(
	() -> {
	new CheckRunGHEventSubscriber(mock(JenkinsFacade.class), mock(SCMFacade.class))
	.onEvent(createEventWithRerunRequest(RERUN_REQUEST_JSON_FOR_PR_MISSING_CHECKSUITE_HEAD_BRANCH));
	})
	.isInstanceOf(IllegalStateException.class)
	.hasMessageContaining("Could not parse check run event:");
	}

[timja]

Have you manually tested this works via ngrok, smee or something?

Code looks good apart from the minor build issues

[acormier1]

Have you manually tested this works via ngrok, smee or something?

Code looks good apart from the minor build issues

Yes, we have this installed in a jenkins instance and it is working as we expect.

[timja]

Looks good pending a green build, (builds are unstable currently due to jenkins-infra/helpdesk#2733)

[codecov]

Codecov Report

Merging #236 (8a464d6) into master (3d152cf) will increase coverage by 0.30%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #236      +/-   ##
============================================
+ Coverage     80.19%   80.49%   +0.30%     
- Complexity      168      170       +2     
============================================
  Files            16       16              
  Lines           520      528       +8     
  Branches         47       49       +2     
============================================
+ Hits            417      425       +8     
  Misses           81       81              
  Partials         22       22              

Impacted Files	Coverage Δ
...ugins/checks/github/CheckRunGHEventSubscriber.java	100.00% <100.00%> (ø)
...s/plugins/checks/github/GitHubChecksPublisher.java	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d152cf...8a464d6. Read the comment docs.

[timja]

Can you check the build please, 2 pmd issues

[acormier1]

Can this be merged at this point?