GitUtils.getValidMergedPRs() returns false positives when PRs are merged into feature branches

[marcaaron]

cc @roryabraham coming from https://expensify.slack.com/archives/C07J32337/p1715047410791299

Problem

When we merged a PR from Rushat it broke the markPullRequestsAsDeployed() part of the staging deploy.

The PR in question had some commits that merged in PRs from other contributors into a feature branch e.g. Merge pull request #12 from software-mansion-labs/travel/terms-and-conditions.

AFAICT we:

1. Get all the commits then pass them to getValidMergedPRs() here
2. Parse the PR # out using a regex

However, in the case where this failed the PR doesn't exist... so we hit a 404 in the next step. I think around here:

App/.github/actions/javascript/markPullRequestsAsDeployed/markPullRequestsAsDeployed.ts

Lines 116 to 120 in 2d96b8a

	const {data: pr} = await GithubUtils.octokit.pulls.get({
	owner: CONST.GITHUB_OWNER,
	repo: CONST.APP_REPO,
	pull_number: prNumber,
	});

Solution

• We could just skip any RequestError with a 404 status...?
• Or figure out how to filter out the PRs that are not merged by Expensify?

[roryabraham]

Thanks for the detailed report. It's been a while since I worked on this code, but I always use TDD in CIGitLogicTests.sh

[roryabraham]

Probably just ignoring 404s is easier to implement. Seems fine - let's log them though.

[marcaaron]

I always use TDD in CIGitLogicTests.sh

Not sure if I followed what you mean there. Gonna raise a PR to catch the RequestError and log, but throw other errors.

[melvin-bot]

This issue has not been updated in over 15 days. @marcaaron eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.3-7 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Handle 404 errors when we have a bad PR referenced in the PR list #41809

If no regressions arise, payment will be issued on 2024-07-10. 🎊

[melvin-bot]

Skipping the payment summary for this issue since all the assignees are employees or vendors. If this is incorrect, please manually add the payment summary SO.

[roryabraham]

I think this is done and no payments are due, closing it out