Check not connected to PR

[hcoles]

If the action is triggered by a pull request then it seem to run, but the check is not associated with the PR and the annotations are effectively lost.

Processing the same input works fine when triggered by push, and the annotations are visible.

Is this expected behaviour, or should triggered by pull request be supported?

[ggrossetie]

Hey @hcoles

I think this is the same issue as #1.
I wish I could associate a check run to a pull request but it seems that this is not possible: https://docs.github.com/en/free-pro-team@latest/rest/reference/checks#runs

As mentioned in #1, the check run (and the annotations) are associated to a commit. Alternatively, we could use octokit.checks.listForRef() but this function is unreliable since it can return more than one result... it will also update an existing check run which is potentially harmful.

Unless I'm missing something this is a GitHub limitation and the alternative solution has benefits but also drawbacks.

My understanding was that the annotations will still be visible on the pull request but apparently that's not true so I might consider to use listForRef on pull requests.

If the action is triggered by a pull request then it seem to run, but the check is not associated with the PR and the annotations are effectively lost.

Could you please provide a link to a run?

[hcoles]

Hi @Mogztter,

I can't provide a run link at the moment as this was in a private repo, but I can recreate something that replicates it publically if the info below does not help.

I'm struggling a bit to understand github's model, but I think the issue is that the sha the action receives is for a merge commit github creates when a PR is opened. This isn't visible via the github GUI, and presumably this is why the annotations are not displayed/associated with the PR.

If I hack the action to change the sha used for a pull request to be github.context.payload.pull_request.head.sha, this looks to give the last commit on the branch being merged in. The annotations then appear against the pull request.

This makes it behave as I'd expect, but I'm unsure if this is the "correct" behaviour.

[ggrossetie]

I can't provide a run link at the moment as this was in a private repo, but I can recreate something that replicates it publically if the info below does not help.

No worries, I've created a repository to test various configuration options.

If I hack the action to change the sha used for a pull request to be github.context.payload.pull_request.head.sha, this looks to give the last commit on the branch being merged in. The annotations then appear against the pull request.
This makes it behave as I'd expect, but I'm unsure if this is the "correct" behaviour.

I think this is the right way to do it. The documentation is not really helpful but I did a few tests and it seems to be working fine.

The github.context.payload.pull_request is only available on a pull request so we can either use github.context.sha (push) or github.context.payload.pull_request.head.sha (pull request).

It's also working nicely when both push and pull_request events are triggered on the same pull request.

I will move forward with your change and do a release 👍

Will you be able to give it a try?

Thanks for your help!

[ggrossetie]

Here's the release: https://github.com/yuzutech/annotations-action/releases/tag/v0.2.1

[hcoles]

Thanks @Mogztter, v0.2.1 is working for me.

The change is definitely an improvement on 0.2.0 (the annotations are no longer lost), but does create some confusing scenarios where you navigate from an annotation to a commit that didn't alter the file. I guess the "correct" behaviour would be to scan through all the commits in the pr and create check runs for each commit with the issues for each affected file, which would be a lot of work if it is even possible.

I'm left wondering whether it's just a bad idea to create annotations on pull requests?

[ggrossetie]

That's a good point but as you said it's not easy to fix.
If you push several commits, GitHub Actions will only run one check run on the last commit and all annotations will be linked to that commit.

I guess the "correct" behaviour would be to scan through all the commits in the pr and create check runs for each commit with the issues for each affected file, which would be a lot of work if it is even possible.

If the annotation has a file (and potentially a line number), we could potentially scan through all the commits to attach it to the latest commit that altered that file (and line number).

I need to give it a try because it would also be confusing if the latest check run is successful but you have a check run on a previous commit that has annotations...

I'm left wondering whether it's just a bad idea to create annotations on pull requests?

I think it makes sense to create annotations on pull requests. In my mind, annotations are a great way to help reviews and contributions so they definitely make sense on pull requests.

I've created a new issue from your comment to track progress 👍