hackforla / website

Hack for LA's website
https://www.hackforla.org
GNU General Public License v2.0
286 stars 700 forks source link

ER: `getModifiedFiles()` in `create-instruction.js` Using Wrong Parameter Causing 404 Error And Workflow Termination #6778

Closed tony1ee closed 2 weeks ago

tony1ee commented 2 weeks ago

Emergent Requirement - Problem

I've noticed Multiple (1, 2) errors in PR GHA checks.

Specifically, the Add Pull Request Instructions workflow. The errors seemed to have started after commit 4295425 was merged in PR https://github.com/hackforla/website/pull/6685.

Looking at the error messages,

Unhandled error: HttpError: Not Found for https\://api.github.com/repos/tony1ee/website/pulls/6777/files

Unhandled error: HttpError: Not Found for https\://api.github.com/repos/del9ra/website/pulls/6776/files

It looks like the workflow was accessing the List pull requests files Github API, but provided incorrect parameter for owner and repo: owner should be hackforla instead of <contributor-github-handle> and repo should be website, as the pull request is for the hackforla/website repo

Specifically, the following API call in github-actions/pr-instructions/create-instruction.js https://github.com/hackforla/website/blob/169f31679915bb460a5efc9e3946af1919d26701/github-actions/pr-instructions/create-instruction.js#L56-L71

Issue you discovered this emergent requirement in

Date discovered

04/28/2024

Did you have to do something temporarily

Who was involved

@tony1ee

What happens if this is not addressed

The PR workflow Add Pull Request Instructions will continue to get 404 Not Found Error when getting modified file list of new PRs and causing confusion.

Resources

Recommended Action Items

Potential solutions [draft]

(As a temporary solution, reverting that merge should prevent new errors from generated and confusing contributors before a fix for this is implemented.)

Based on [GitHub Webhooks] Webhook payload object for pull_request , we should supply the owner and repo parameter with names obtained from base instead of head of the pull_request object, see proposed replacement for line 58 and 59:

const repoName = context.payload.pull_request.base.repo.name; 
const ownerName = context.payload.pull_request.base.repo.owner.login; 
t-will-gillis commented 2 weeks ago

Hey @tony1ee Great catch regarding the base versus head , and thanks for the proposed replacement code for lines 58 and 59.

@anthonypz since you worked on PR #6685 fixes 5812, please let us know if you would like to continue with the fix as well.

t-will-gillis commented 2 weeks ago

Closing this ER as resolved- changes were picked up in PR #6790 and the posting of PR instructions appears to be working (will continue to monitor)