PMD annotations are added in an (Unknown event) in a (Unnamed workflow) when used in a pull request build

[n-peugnet]

Hi, I tried to use your action but I found a bug on pull_request builds. It took me some builds to find out that the action was correctly reporting violations but not on the correct build.

On my last attempt tried to use this option of the checkout action but strangely the check was still added to the merge commit :

- uses: actions/checkout@v2
  with:
    ref: ${{ github.event.pull_request.head.sha }}

I looked a little bit at your code and I wonder if using context.ref when the check is created instead of context.sha would solve this problem (when the above option is set in the checkout action).

[jwgmeligmeyling]

Hi, I noticed this myself too (see the same issue here as well: jwgmeligmeyling/spotbugs-github-action#2 ).

I don't think this action should be used on the pull_request event. This is because the pull request executes against a merge commit, and the line numbers for this merge commits will be incorrect. So the annotations would be put in the wrong place if github.event.pull_request.head.sha were used.

I am just going to encourage users to use the push event instead and add an event based filter to the example configuration.

For example;

      - uses: jwgmeligmeyling/spotbugs-github-action@master
         if: ${{ github.event_name == 'push' }}
         with:
           path: '**/spotbugsXml.xml'

       - uses: jwgmeligmeyling/pmd-github-action@master
         if: ${{ github.event_name == 'push' }}
         with:
           path: '**/pmd.xml'

       - uses: jwgmeligmeyling/checkstyle-github-action@master
         if: ${{ github.event_name == 'push' }}
         with:
           path: '**/checkstyle-result.xml'

I could consider though to still push the result to github.event.pull_request.head.sha for the pull_request event and then just not push the annotations at all, but only do that for the push event. But the annotations for the pull_request event will always be meaningless.

[n-peugnet]

I don't think this action should be used on the pull_request event. This is because the pull request executes against a merge commit, and the line numbers for this merge commits will be incorrect.

In fact when I add the above ref parameter to the checkout action the test are not run on a merge commit but on the real HEAD of the pull request. This way the line numbers are correct. So with the fix mentioned on the other repo it would be perfect. The checkout action ref parameter could be added in the documentation.

You can see on this build that for the check job, both push and pull_request events correctly built against the right HEAD commit: PolyProcessInterface/ppi@670f31b

[jwgmeligmeyling]

Yes allright, but it defeats the purpose of a build on the pull_request event: most users will use this event to verify that the changes are still OK after merging into master. So I could point users to that option in the documentation, but it won't be one size fits all...

I by the way cant use the ref either: that will be the branch name , which can't be use to push the check result to through the API.

[n-peugnet]

Yes this is why I made two jobs, one for the tests on the results of the merge and one for the codestyle on the head of the pull request. But I don't want to trigger this action on each push on every branch. I want to keep the trigger only on pull requests to master.

I mean, as you said, running this type of checks on a merge does not make a lot of sense but the reason why I run them on the pull request is only to limit the number of builds.

[jwgmeligmeyling]

So then you're suggestion would be to document these two approaches in the README?

Or would you still see a way to properly handle this programatically?

[n-peugnet]

First, if we want to make this action work on pull request builds, the fix you mentioned in the linked issue must be added :

let sha = github.context.sha

if (github.context.payload.pull_request) {
    sha = github.context.payload.pull_request.head.sha
}

Then you should add a warning in the readme that if someone wants to run this action on the pull request event but check on the HEAD instead of the merge, the following option must be added to the checkout action.

- uses: actions/checkout@v2
  with:
    ref: ${{ github.event.pull_request.head.sha }}

[jwgmeligmeyling]

Hmm I would expect github.context.sha == github.event.pull_request.head.sha if ref: ${{ github.event.pull_request.head.sha }} is set.... Apparently not 🤷

Is there a way to reference the SHA for the checkout out ref?

So that we can still use github.context.sha if ref: ${{ github.event.pull_request.head.sha }} is not set.

[n-peugnet]

Hmm I would expect github.context.sha == github.event.pull_request.head.sha if ref: ${{ github.event.pull_request.head.sha }} is set.... Apparently not

haha yes this is what I was also surprised to find out !

Is there a way to reference the SHA for the checkout out ref?

You mean in the parameters of the checkout action ? It does not seem so.

[jwgmeligmeyling]

Allright. I'll make the suggested change later tonight (CEST). Thanks for your detailed report!

I'll also make the change available to the actions for Checkstyle and SpotBugs.

[jwgmeligmeyling]

@n-peugnet Would you mind a solution where the ref can be passed as an input, just as with checkout? This as a workaround until there's a neat solution to find the actual ref checkout by the checkout action.

I'd really like to avoid issues where users report their annotations are reported on the wrong line number for pull request events (that do not checkout the pull request ref without the additional configuration you provided).

[n-peugnet]

That would be ok. But people will continue to have the (Unknown event) problem as long as it is not mentioned in the README that there is a specific configuration to apply when you want to use this action on pull request build.

[jwgmeligmeyling]

I'm still working on an other issue: the results are posted under the "push" event, but are generated from the "pull_request" event. Figuring out how to tackle that. I suppose it uses check suites under the hood, but the API on registering check runs for a specific check suite is not documented (using https://developer.github.com/v3/checks/runs/#create-a-check-run as resource). I'll keep you posted...

[jwgmeligmeyling]

Hmm after some trying out, this is going to be hard to resolve. I filed an issue on the forums: https://github.saobby.my.eu.orgmunity/t/specify-check-suite-when-creating-a-checkrun/118380?u=jwgmeligmeyling .

Meanwhile I am going to apply your patch. Just a heads up that there will remain a few quirks when you have multiple workflows or events your workflow listens on. It will just pick one workflow to place the results under.

Its running a bit late, so I'll finish the patch tomorrow.

[n-peugnet]

Just a heads up that there will remain a few quirks when you have multiple workflows or events your workflow listens on. It will just pick one workflow to place the results under.

I can live with that haha !

Its running a bit late, so I'll finish the patch tomorrow.

Ok great ! Of course take your time it's not that urgent.

[jwgmeligmeyling]

I've implemented the suggested fix in #5 and described the other issue in #4 .

[n-peugnet]

Thank you it is working exactly as intended !