jenkinsci/github-checks-api

GSoC 2020 - GitHub Checks API for Jenkins Plugins. https://www.jenkins.io/projects/gsoc/2020/projects/github-checks/

Bill Collins

@mrginglymus

Of course I'd also like to be able to unstable within a withChecks, but I suspect, worms, can of etc

Bill Collins

@mrginglymus

So I worked out how to do that (horribly), but then of course realised there's nothing special that can be detected about unstable, so it'd just pick up all unstables.

Kezhi Xiong

@XiongKezhi

One of the next targets on my personal checks-api hitlist was input steps - automatically setting ACTION_REQUIRED when input is requested, and resolving with details of input provided. However, I wasn't quite sure how to slice this. I could see three ways (dependent on what APIs are actually available...)

Add a 'checksName' to input in pipeline-input-step

Make pipeline-input-step withChecksName aware

Implement an input listener (does one even exist?) in checks-api and let that do the heavy lifting

it's better to let other plugins dependent on the check API to resolve the context themselves not the other way around IMO

Kezhi Xiong

@XiongKezhi

Of course I'd also like to be able to unstable within a withChecks, but I suspect, worms, can of etc

maybe unstable checks should also be handled by the consumers as well? since we have no way to know what conclusion to publish in withChecks on failure of steps.

Bill Collins

@mrginglymus

Yes, i decided no 2 was the most sensible option. The same applies to the unstable step i suspect; i will work on POC for that too.

Bill Collins

@mrginglymus

The coverage-api plugin step appears to be a 'classic' build step, and so I can't see how to get the context to get the current checks details.

Kezhi Xiong

@XiongKezhi

seems it just add a symbol to the build step instead using a pipeline step and execution, don't what's relation between them...

@mrginglymus have you tried using github-checks with the updating feature in a freestyle project?

I got an exception

java.lang.UnsupportedOperationException
    at java.util.Collections$UnmodifiableCollection.add(Collections.java:1057)
    at hudson.model.Actionable.addAction(Actionable.java:155)
    at io.jenkins.plugins.checks.github.GitHubChecksContext.addActionIfMissing(GitHubChecksContext.java:127)
    at io.jenkins.plugins.checks.github.GitHubChecksPublisher.publish(GitHubChecksPublisher.java:78)
    at io.jenkins.plugins.checks.status.BuildStatusChecksPublisher.publish(BuildStatusChecksPublisher.java:38)
    at io.jenkins.plugins.checks.status.BuildStatusChecksPublisher.access$100(BuildStatusChecksPublisher.java:32)
    at io.jenkins.plugins.checks.status.BuildStatusChecksPublisher$JobCheckoutListener.onCheckout(BuildStatusChecksPublisher.java:124)
    at hudson.model.AbstractBuild$AbstractBuildExecution.defaultCheckout(AbstractBuild.java:579)
Caused: java.io.IOException
    at hudson.model.AbstractBuild$AbstractBuildExecution.defaultCheckout(AbstractBuild.java:581)
    at jenkins.scm.SCMCheckoutStrategy.checkout(SCMCheckoutStrategy.java:86)
    at hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:499)
    at hudson.model.Run.execute(Run.java:1880)
    at hudson.model.FreeStyleBuild.run(FreeStyleBuild.java:43)
    at hudson.model.ResourceController.execute(ResourceController.java:97)
    at hudson.model.Executor.run(Executor.java:428)

And I think we might have made a mistake in current implementation of the updating feature: the GitHubChecksAction should not be an job- or project-wise action, it should be a run- or build-wise one.

Kezhi Xiong

@XiongKezhi

And since we didn't possess the build while publishing the checks, we may just have to query the check run from github

Bill Collins

@mrginglymus

Rats. Perhaps for freestyle builds we should not be auto-updating.

There would only be one that would need to update, which is the top-level one (that's triggered on queue, checkout, etc)

Kezhi Xiong

@XiongKezhi

There would only be one that would need to update, which is the top-level one (that's triggered on queue, checkout, etc)

top-level one? I don't get it.

And another problem to attach action to the job is that, image we have two builds for commit A and B, and they are triggered accordingly in a short interval. At the start, A creates the action with the check run id for A; then B starts publishing, since we attached the action to the job, the action for A is also visible to B. Thus, B will reuse the check run for A and publish to that check run.

Bill Collins

@mrginglymus

Ah, sorry, I see what you mean re job/run actions

Bill Collins

@mrginglymus

I'm just popping out for a walk but - it looks like we can make GitHubChecksContext always accept a run (promoting from GitSCMChecksContext) and accept that in some limited circumstances it may be null.

As far as I can tell that'll only be when it's created because of a 'queued' listener.

We then hook our actions onto the Run, not the Job, and if we don't have a Run we just say "Sorry, don't know what the ID is"

Kezhi Xiong

@XiongKezhi

I tried to query the checks from github (see jenkinsci/github-checks-plugin#98) which requires minimal change but we can't reply on that :( since when re-schedule a build for a same commit, the checks is already existed and we'll update it, then the duration consists of two builds.

Bill Collins

@mrginglymus

Hmm, and we can't use the job url because that would also be the same when it's in the 'queued' state.

Bill Collins

@mrginglymus

jenkinsci/github-checks-plugin#99

Oh wait, I can simplify that even more

Done

Kezhi Xiong

@XiongKezhi

Hmm, and we can't use the job url because that would also be the same when it's in the 'queued' state.

That's ok, we discussed that earlier and can't find a better choice than job url.

Bill Collins

@mrginglymus

Right I think I have something that works. The only limitation I think is that we will currently overwrite rather than update the QUEUED status checks with the IN_PROGRESS checks for pipeline jobs as we won't have anywhere to store that information, but I think that's reasonable.

Kezhi Xiong

@XiongKezhi

nothing will be published when the build is still queued :+1:

Kezhi Xiong

@XiongKezhi

and the queued time will not be included which is good

Bill Collins

@mrginglymus

Yeah, that makes more sense

Ullrich Hafner

@uhafner

While looking at jenkinsci/warnings-ng-plugin#721 I am wondering if the current implementation can be improved: Now every consumer of the checks API needs to provide an if-elseblock to check for a context object and set the name accordingly. Wouldn’t it be smarter if that decision would be solely made by the checks plugins? As far as I understand the current implementation the checks API looks for withChecks blocks and then adds a corresponding action to the run. That means the checks plugin can decide on its own if the name of the consumer should be used or the name that is part of the action.

Bill Collins

@mrginglymus

That would remove the ability of a consumer to be able to override a withChecks block, although I can't immediately think of a need to be able to do that.

Ullrich Hafner

@uhafner

Hmm, maybe for that usecase we can add an additional API method. Something like publisher.forceName or so. Then we would not need to change any of the consumers.

Bill Collins

@mrginglymus

As far as I understand the current implementation the checks API looks for withChecks blocks and then adds a corresponding action to the run.

I don't think that's right - it adds a POJO to the body invoker context, which is going to be difficult (impossible?) to get when we create or invoke a publisher.

Kezhi Xiong

@XiongKezhi

The action is not added to the run but to the StepContext which is only visible in the withChecks block.

I think the initial reason why we introduced it is that: if there are parallel stages or multiple stages in a pipeline, for example, and they both invokes publishIssues for PMD report, these reports will be mixed in a single check run (or worse if the implementation didn't provide update mechanism, the later one will just overwrite the former). The solution can be:

the warnings-ng plugin or other consumer plugins add a field to allow user specific the name (maybe for each tool in warnings-ng).
Provide the withChecks step.

By publisher.forceName, you mean after this invocation, all checks (published by consumers' or publishChecks step) should all use the name specified in forName step?

Ullrich Hafner

@uhafner

I see. That means that the checks API has no way to decide if it is called withing the withChecks scope.

This is something only the consumer knows.

Kezhi Xiong

@XiongKezhi

yep

Tim Jacomb

@timja

this PR by @mrginglymus is adding details on where a build failed to the overall build check: jenkinsci/checks-api-plugin#66, any feedback on it UI / behaviour would be much appreciated

Bill Collins

@mrginglymus

I'm trying to work out the correct way to get build logs for a step - as far as I can tell it's something like:

LogStorage logStorage = LogStorage.of(flowNode.getExecution().getOwner());
AnnotatedLargeText<FlowNode> logText = logStorage.stepLog(flowNode, true);

and then do the necessary BAOS wrangling on logText.

Is that right? And if so, what runes do I need to allow for LogStorage still being in beta?

or should I be going via flowNode.getAction(LogAction.class).getLogText()?

Tim Jacomb

@timja

<useBeta>true</useBeta> in the properties of the pom.xml @mrginglymus

hmm that doesn't look the most efficient

actually yeah it's fine

https://github.com/jenkinsci/workflow-api-plugin/blob/cc1cb400f43f8478f0180a679e2fe470b1e089f0/src/main/java/org/jenkinsci/plugins/workflow/log/FileLogStorage.java#L255

Bill Collins

@mrginglymus

So I've finally got to using withChecks in anger at work, and hit a bug which has (at least) two solutions

https://github.com/jenkinsci/checks-api-plugin/blob/master/src/main/java/io/jenkins/plugins/checks/steps/WithChecksStep.java#L191-L193 is lacking a title, which makes it illegal as far as GitHub is concerned

We can either add a title here, or, we can make github checks plugin more resiliant to it missing and just use the name of the checks as the title of the output if non is given

Ah, it also is lacking a summary (which is also requried).

So probably the former solution.

Bill Collins

@mrginglymus

Oh no, the summary is there isn't it.

jenkinsci/checks-api-plugin#69

Where communities thrive

People

Repo info

Activity