Comments (16)
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.
from pmd-github-action.
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.
from pmd-github-action.
Thank you it is working exactly as intended !
from pmd-github-action.
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.
from pmd-github-action.
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
from pmd-github-action.
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.
from pmd-github-action.
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.
from pmd-github-action.
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?
from pmd-github-action.
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 }}
from pmd-github-action.
Hmm I would expect
github.context.sha == github.event.pull_request.head.sha
ifref: ${{ 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.
from pmd-github-action.
@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).
from pmd-github-action.
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.
from pmd-github-action.
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...
from pmd-github-action.
Hmm after some trying out, this is going to be hard to resolve. I filed an issue on the forums: https://github.community/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.
from pmd-github-action.
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.
from pmd-github-action.
I've implemented the suggested fix in #5 and described the other issue in #4 .
from pmd-github-action.
Related Issues (5)
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from pmd-github-action.