[Hold for Payment on July 22] OSBotify falsely marks pull requests as cherry-picks instead of standard deploys

[roryabraham]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Ensure that the open StagingDeployCash checklist issue does not have the 🔐 LockCashDeploys 🔐 label.
2. Merge a pull request.
3. Once the staging deploy for that pull request has begun, add the 🔐 LockCashDeploys 🔐 label to the open StagingDeployCash

Expected Result:

Once the staging deploy for the pull request has completed, OSBotify should comment on the pull request saying that it was "Deployed to staging", with the normal table and such, like this:

Actual Result:

Once the staging deploy for the pull request has completed, OSBotify comments on the pull request saying that it was Cherry Picked to staging, like this:

Workaround:

Not applicable - it's just a misleading comment and doesn't affect app usability.

Platform:

Github only

Version Number: 1.0.63-0
Logs: https://github.com/Expensify/Expensify.cash/actions/runs/906930849

View all open jobs on Upwork

[MelvinBot]

Triggered auto assignment to @JmillsExpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[roryabraham]

This can be worked on by an external contributor, but they'll need help from an Expensifier to test their PR.

[roryabraham]

Ultimately this race condition is occurring because right here we are making the incorrect assumption that, when a staging deploy completes, if the StagingDeployCash is locked, then the PR that was deployed must have been cherry-picked.

There are a number of potential ways one could determine if the deploy was the result of a normal staging deploy vs a cherry-pick, and I'm not sure which is best. One idea would be to check if the StagingDeployCash was locked at the time the pull request was merged.

[JmillsExpensify]

Sorry @roryabraham I've been super busy with an internal project. Picking this one up tomorrow.

[roryabraham]

No worries, this one is not that urgent.

[JmillsExpensify]

Done! Upwork job is here: https://www.upwork.com/jobs/~017e5b710bf91a779e

A note for contributors:

If you are interested in working on this issue, please post directly in this issue with a proposal with the technical explanation of the changes you will make. Somebody from Expensify will review your proposal, and if your proposal is accepted, you will be hired in Upwork for the job.

[JmillsExpensify]

Increased the payout on Upwork. Hopefully we'll get some proposals here soon!

[attaradev]

@JmillsExpensify and @roryabraham I can investigate this.

[attaradev]

My proposal

• Create a separate job for checking the presence of the lock which will run first, and set that as the output value for the job
• Pass the output value as the IS_CHERRY_PICK for the comment job

@roryabraham what do you think?

[roryabraham]

@mikeattara When would this job execute?

[attaradev]

I can start working on it immediately, @roryabraham

[roryabraham]

Sorry @mikeattara, I don't think your proposed solution will work. Placing a separate job at the start of the platformDeploy.yml workflow won't resolve the race condition:

1. Merge a PR, and preDeploy will begin.
2. Then add the lock label
3. By the time your separate job begins after the ~10-30 second startup time, the label will be present.

[parasharrajat]

I am not sure about the cherry-picking concept here. What I understand so far is that in the following conditions a PR is CP.

1. If CP staging label is present before PR is merged.
2. As per the current issue, If we don't have the LockCashDeploys label on the StagingDeployCash issue.

So I can't think of any solution other than what you have posted above. So If we follow your solution then this would be achieved as below

1. We get the StaginDeployCash using GithubUtils.getStagingDeployCash().
2. Get a set of [added, removed] pairs for the LockCashDeploys label for StaginDeployCash using paginated https://octokit.github.io/rest.js/v18#issues-list-events.
3. 1-2 will run as a single step where outPut will be the timeline set.
4. Now inside the markPullRequestsAsDeployed we have to get each PR one by one to get their merged_at time using https://octokit.github.io/rest.js/v18#pulls-get.
5. Now merged_at time comes between any of the [added, removed] pairs then we mark as CP, otherwise normal staging deploy.

For Pairs => [added, removed(if not present then take as now)]. We will be needing pagination https://octokit.github.io/rest.js/v18#pagination as well.

if you can post other possible solutions then I can find the optimal one out of those.

[roryabraham]

@parasharrajat I think your solution would work well, with one modification:

5. If the merged_at time comes between any of the [added, removed] pairs and the PR has the CP Staging label, then it was a CP. Otherwise it's a normal staging deploy.

[roryabraham]

@parasharrajat Want to take on this issue and #3662 at the same time? 🙃 Since we need pagination for this, we might as well build a more general-purpose paginator for the GitHub API

[parasharrajat]

1. If the `merged_at` time comes between any of the `[added, removed]` pairs **and the PR has the `CP Staging` label**, then it was a CP. Otherwise, it's a normal staging deploy.

Oh yeah. So I think I need to add one more step that is PR has a CP staging label before Pr was merged then Its CP.

[parasharrajat]

But it seems that https://octokit.github.io/rest.js/v18#pagination this gives all the results autmatically. Let me read that issue and see what is required there.

[roryabraham]

I need to add one more step that is PR has a CP staging label before Pr was merged then Its CP

Well, if a PR is merged with the CP Staging label when the StagingDeployCash is unlocked, then it goes through a regular staging deploy (just like any other E.cash PR that's merged when the StagingDeployCash is unlocked).

[parasharrajat]

OK, then your point makes sense. I told you I am not yet fully familiar with CP flow. 😉

[roryabraham]

@JmillsExpensify Let's get @parasharrajat hired for this on Upwork.

[JmillsExpensify]

Hired! Looks like we are testing the PR on the latest QA issue, so I'll make sure to pay you out as soon as that goes through as well! Or let me know if I've misunderstood the status!

[roryabraham]

As stated in the PR, this did cause a regression that will need to be fixed.

[parasharrajat]

@roryabraham Please update here if regression is fixed. Thanks.

[roryabraham]

Hey @parasharrajat! The regression seems to have been fixed, but in order to really test if this issue is complete, we'll need to:

1. Ensure that the open StagingDeployCash checklist issue does not have the 🔐 LockCashDeploys 🔐 label.
2. Merge a pull request.
3. Once the staging deploy for that pull request has begun, add the 🔐 LockCashDeploys 🔐 label to the open StagingDeployCash
4. Wait for the staging deploy to finish, and verify that it gets a "deployed to staging" comment and not a "cherry-picked to staging" comment.

[roryabraham]

@parasharrajat We can sync 1:1 and test that on the next open checklist?

[parasharrajat]

Sure. Thanks

[roryabraham]

Testing with #4096

[roryabraham]

Reopening because there were some very weird and unrelated failures in the standard deploy from the dummy PR.

[roryabraham]

The dominos are lined up. We just need to wait for this deploy to finish and then verify that this PR gets the "Deployed to staging" comment instead of "Cherry-picked to staging"

[roryabraham]

QA passed, closing this out!

[parasharrajat]

@JmillsExpensify Any update on Upwork? Thanks

[JmillsExpensify]

Yep! Closed out. Thank you