Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Activity
  • 10:55
    uhafner commented #71
  • 10:14
    timja commented #71
  • 09:25
    XiongKezhi commented #71
  • 09:19
    timja commented #49
  • 08:57
    XiongKezhi commented #49
  • 03:15
    almostjulian commented #49
  • Jan 15 21:28
    agaudreault-jive edited #71
  • Jan 15 21:28
    agaudreault-jive opened #71
  • Jan 15 20:56
    agaudreault-jive commented #46
  • Jan 15 20:41
    agaudreault-jive commented #70
  • Jan 15 12:46
    rmathewsbeyond edited #70
  • Jan 15 12:46
    rmathewsbeyond opened #70
  • Jan 14 17:48
    agaudreault-jive commented #105
  • Jan 14 17:46
    agaudreault-jive review_requested #105
  • Jan 14 17:46
    agaudreault-jive ready_for_review #105
  • Jan 14 15:37
    agaudreault-jive synchronize #105
  • Jan 14 15:12
    agaudreault-jive synchronize #105
  • Jan 14 14:51
    agaudreault-jive synchronize #105
  • Jan 14 02:28
    XiongKezhi commented #104
  • Jan 13 15:34
    agaudreault-jive edited #104
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...)

  1. Add a 'checksName' to input in pipeline-input-step
  2. Make pipeline-input-step withChecksName aware
  3. 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
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:

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

  2. 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
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
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.