Ability to reuse/update checks

[mrginglymus]

Coming from jenkinsci/junit-plugin#210...

We're making extensive use of the checks API now, and so far it's absolutely fantastic. The final missing piece of the jigsaw is the ability to update and reuse checks.

We have moved from reusing statuses, where we would ping a 'queued', 'in progress', and 'success'/'failure' message as appropriate, as tests progressed.

With checks, we are unable to do this (properly) via the provided API, and so results only get reported when a task is complete. We could publish a new check with the same name, but then we lose things like duration.

My ideal API would look something like:

withCheck('My Check Name') { check ->
    def result = sh script: 'build', returnStdout: true
    check.success(message: result)
}

and, for use with another plugin

withCheck('My Check Name') { check ->
    sh 'test --output output.xml'
    junit testResults: 'output.xml', check: check
}

where the implementation of withCheck is (in psuedo code) something like

void withCheck(String name, Closure body) {
    def check = createCheck(name)
    try {
        body(check)
    catch (FlowInterruptedException f)  {
        check.abort()
        throw f
    }
    catch (Exception err) {
        check.fail()
        throw err
    }
    if (!check.complete) {  // ie, only call success if nothing else has already 'closed' the check
        check.success()
    }
}

[XiongKezhi]

would it make sense if I expose the status instead of setting it as completed (we can use that as default if the user didn't provide the status)?

Since we don't have the mechanism to track the status of a check unless we request GitHub through project name, PR, and check name which is bad IMO.

[jglick]

Steps cannot take closures with arguments. (Even if they could, such a syntax would be incompatible with Declarative Pipeline.) You can add either an environment variable or a custom contextual object that other steps can look for:

withCheck('My Check Name') {
    sh 'test --output output.xml'
    junit 'output.xml'
}

[mrginglymus]

Ah yes, sorry - spent too much time on shared libraries - where what I've written is roughly what I implemented before I realised it woudln't really work without more plugin support.

A contextual checks object would make a lot of things easier.

[jglick]

If interested, check Javadoc.

[mrginglymus]

Since we don't have the mechanism to track the status of a check unless we request GitHub through project name, PR, and check name which is bad IMO.

Yes, that wouldn't be ideal! Can we not add the id returned by the checks provider to the ChecksDetails? It doesn't seem an unreasonable assumption that any checks provider would provide a unique id to allow updating the check. I see that github-api is lacking an api to update an existing check, beyond the internal implementation of paginating annotations. That looks like it should at least be a fairly simple addition to make.

[jglick]

update an existing check

There is no need for a special API. You just submit a check with the same name and different details.

[mrginglymus]

Ah, does that work? I tried to find details of what happens in the github docs and it wasn't clear. I note that github-api does explicitly call the update endpoint to append annotations.

[XiongKezhi]

Ah, does that work? I tried to find details of what happens in the github docs and it wasn't clear. I note that github-api does explicitly call the update endpoint to append annotations.

That's how our status checks work.

[mrginglymus]

I believe you, I just wish the documentation was a bit clearer :) For instance I wonder what it does to existing annotations?

[XiongKezhi]

After reading so many conversations about the custom names for JUnit checks, I'm still a little confused at some points.

You create this issue here just want us to provide the ability to update the checks, which you can actually do by publishing checks with the same name as long as you call the publishChecks in the same branch. Does that meet your needs? Or you need more to publish different status to help you keep track of your builds, since now publishChecks will only publish completed checks?

And about the withChecks step, you write the closure because you tend to use it to let it help you create or update the checks published by others plugins? I currently have no idea how to implement it (I'm not quite familiar with the interactions between the pipeline code and java code), since in my understanding, shouldn't such works are achieved by shared libraries?

[mrginglymus]

So I did initially try just creating a checks with the same name, however, this didn't work as I would hope as, because every time time the checks api posts a new check to github it clobbers the started at time:

https://github.com/jenkinsci/github-checks-plugin/blob/26c5abfa13b47289f28df94583dabf33901eb6ce/src/main/java/io/jenkins/plugins/checks/github/GitHubChecksPublisher.java#L88

For almost every other field this doesn't matter, as it's the last value that matters (title, status, details, finished at etc etc) but for started at the only thing that knows it is the first check I post. The junit plugin (currently) has no way of knowing what time the check was started, and unless I'm missing something undocumented in the github docs, when we post a new check it entirely replaces the old check.

[timja]

the suggestion is that adding a new object that contains checks context information would allow other implementations to resolve the name from that object rather than using a default.

i.e.:

withChecks('my junit') {
  // this adds a ChecksInfo or something which has a name
  junit '*.xml'
}

and junit would report 'my junit'

which woud mean there would be a standard way to provide a checks name (although a default would still be required)

and yes ah Bill mentioned it would allow publishing the in progress check as well

[jglick]

it clobbers the started at time

Well, not sure if the junit plugin even defines otherwise (by default a check would be considered instantaneous), but if so, then it should keep track of the time when the check was first added under a given name, and use that same explicit start time.

[jglick]

Or if for architectural reasons it is impossible for Jenkins to keep track of this information (the earlier check was posted by a different plugin, for example), github-checks could at your option track the start time of a given build × check name in some InvisibleAction so that an update could be displayed as occupying an interval of time.

[XiongKezhi]

I'm figuring out a way to update the checks.

We need an update method:

public void update(ChecksDetails addedDetails);

or

public void update(CheckDetails originDetails, ChecksDetails addedDetails);

since the dependent github-api library doesn't support updating the checks either (which always POST instead PATCH as github documented for updating a check), we just append things (started at time, annotations) in addedDetails to originDetails and publish another again.

And another problem is, where we define this method, in checks-api, github-check, or just in the withChecks step?

@timja @jglick

[mrginglymus]

I've opened a PR against github-api to allow for updates: hub4j/github-api#980 - it appears to be a very small change required to get it to work.

[mrginglymus]

This was fixed by #49