Fix #128 : On push, use files in commit to avoid "not found" error #130
Conversation
If there is no prior commit then the compare call will fail.
lib/util.js
Outdated
| const compareResponse = await octokit.rest.repos.compareCommitsWithBasehead({ | ||
| const commitResponse = await octokit.rest.repos.getCommit({ | ||
| ...context.repo, | ||
| basehead: `${eventData.before}...${eventData.after}`, | ||
| ref: eventData.after, |
There was a problem hiding this comment.
This change will now only consider the head commit of the commits that have been pushed. If multiple commits have been pushed at once, then the file list won't be complete.
The push event is described here: https://docs.github.com/en/developers/webhooks-and-events/webhooks/webhook-events-and-payloads#push
The sample payload seems to be for a tag, that has been pushed - interestingly it also has before=00000.... That means, the same problem (#128) should occur for if a tag has been pushed.
The payload also contains an array commits[] - in the example it's empty. I guess this is because it's a tag, that has been pushed. But for usual pushes, that should contain all the commits that are contained in the push. If there is no previous commit (before=0000....) we maybe need to loop through the commits and request each commit with your suggested method (getCommit).
Also there are other interesting flags in the push event: e.g. if a branch/tag is deleted, we would also get a push event and likely fail to determine the modified files.
There was a problem hiding this comment.
Thanks for the clarification on the push event payload. I misunderstood and thought the before was simply the immediate prior commit to what after represented. But it seems GitHub webhooks may be trying to be efficient by collapsing multiple commits into a single payload.
I'll reconsider my approach for handling the specific instance when before=0000.... per your recommendations.
tests/util.test.js
Outdated
| // Personal Access Token to perform tests with. | ||
| // https://github.com/settings/tokens/new | ||
| const token = process.env['TEST_GITHUB_TOKEN']; | ||
| if (!token) { | ||
| throw new Error('Set `export TEST_GITHUB_TOKEN="<your token>"` to run tests'); | ||
| } | ||
|
|
There was a problem hiding this comment.
This change will break the CI build - as there is no env var "TEST_GITHUB_TOKEN" defined.
For the unit tests, no real token is required (hence I used my_test_token), because all the requests to the github api are mocked using nock. Since you added a new API call, we likely just need to mock this as well.
It seems, you updated the mock - so I guess this change here is not needed anymore.
There was a problem hiding this comment.
Ah, understood. I'll revert this change. Thanks
tests/util.test.js
Outdated
| @@ -294,16 +301,16 @@ describe('pmd-github-action-util', function () { | |||
| process.env['GITHUB_EVENT_NAME'] = 'push'; | |||
| process.env['GITHUB_EVENT_PATH'] = __dirname + '/data/push-event-data.json'; | |||
| nock('https://api.github.com') | |||
| .get('/repos/pmd/pmd-github-action-tests/compare/44c1557134c7dbaf46ecdf796fb871c8df8989e4...8a7a25638d8ca5207cc824dea9571325b243c6a1?per_page=30&page=1') | |||
| .get('/repos/pmd/pmd-github-action-tests/commits/8a7a25638d8ca5207cc824dea9571325b243c6a1?per_page=30&page=1') | |||
| .replyWithFile(200, __dirname + '/data/compare-files-page1.json', { | |||
There was a problem hiding this comment.
Please also rename this file "compare-files-page1.json" and update the contents - it's now not anymore the response from compare, but from commits, which potentially looks different.
You can just look at https://api.github.com/repos/pmd/pmd-github-action-tests/commits/8a7a25638d8ca5207cc824dea9571325b243c6a1?per_page=30&page=1 to get the json.
Note: I've simplified the original response for compare back then to make it smaller and contain only the essential parts - and modified the files part to contain the data I needed for tests.
Fixes #128
Changes
lib/util.jswhen determining the modified files from a push event, now uses octokit'sgetCommitmethod instead ofcompareCommitsWithBaseheadto avoid errors when the push event does not include a "before" commit and only an "after" commit.getCommitmethod only includes files that were part of the commit, so it naturally identifies the files that would be part of the diff when compared to the immediately prior commit.tests/util.test.js, pulls test token from environment variableTEST_GITHUB_TOKENto mitigate accidentally committing it (like I did in my prior PR)npm run allTests