App icon indicating copy to clipboard operation
App copied to clipboard

[$250] Race condition with the CP Staging label

Open roryabraham opened this issue 3 years ago • 20 comments

Coming from https://github.com/Expensify/App/pull/7255 ...

Problem

There exists a race condition when the CP Staging is applied to a pull request after it's merged. Both of the following are possible:

  1. When the merged pull request is fetched here, it already has the CP Staging label, and is deployed to staging.
  2. When the merged pull request is fetched in the location listed above, it does not have the CP Staging label, and is not deployed to staging.

In both scenarios, a comment that looks like this will be left on the pull request:

image

Why this is important

In the second scenario, this is confusing because the expected behavior/whether or not the pull request was CP'd to staging is unclear.

Solution

Update the warnCPLabel.yml workflow to tailor its comment based on whether or not the CP Staging label was applied to the pull request when it was already merged.

  • If the pull request not yet merged, keep the same comment.
  • If the pull request is already merged, use the following comment:
    1. Find the preDeploy.yml workflow run for the pull request.

    2. List jobs for that workflow run and find the skipDeploy job.

    3. If the conclusion of that job is not resolved, poll the API until it is resolved.

    4. If the conclusion is success or something else, use the following comment:


      :warning: Heads up! :warning: Since the CP Staging label was applied after this PR was merged, it was not CP'd to staging. If you need it to be deployed to staging, tag a member of @Expensify/mobile-deployers to CP it manually.


    5. If the conclusion is skipped, use the following comment:


      :warning: Heads up! :warning: The CP Staging label was applied after the PR was merged. This leads to unpredictable behavior. In this case this PR will be deployed to staging, but to guarantee this in the future be sure to apply the CP Staging before merging the pull request.


roryabraham avatar Jan 17 '22 19:01 roryabraham

No update here yet.

roryabraham avatar Feb 18 '22 22:02 roryabraham

This is pretty low priority, no update.

roryabraham avatar Mar 23 '22 04:03 roryabraham

Again no update.

roryabraham avatar Apr 25 '22 18:04 roryabraham

No update – this isn't that important but would be nice polish and hopefully pretty easy.

roryabraham avatar Jun 01 '22 19:06 roryabraham

Not a priority

roryabraham avatar Jul 07 '22 17:07 roryabraham

No update

roryabraham avatar Aug 08 '22 17:08 roryabraham

No update

roryabraham avatar Sep 29 '22 15:09 roryabraham

No update

roryabraham avatar Oct 31 '22 16:10 roryabraham

Triggered auto assignment to @mateocole (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] avatar Nov 02 '22 13:11 melvin-bot[bot]

@roryabraham removed and re-added the bug label to get a BZ team member added who can help move this forward.

puneetlath avatar Nov 02 '22 13:11 puneetlath

@puneetlath is this something that can be marked as external and I can make a job for in Upwork?

mateocole avatar Nov 07 '22 16:11 mateocole

@mateocole no I think this will need to be handled internally by @roryabraham or someone else.

puneetlath avatar Nov 08 '22 20:11 puneetlath

Is this actually a bug or is it a new feature? If it's a bug, is it reproducible and can it therefore be assigned to the demolition team? @mateocole can you look into this and figure out what needs to be done to push this forward?

arielgreen avatar Nov 08 '22 22:11 arielgreen

@arielgreen looking at this from a P/S statement it looks like a feature but I'll wait for @roryabraham to clarify

mateocole avatar Nov 09 '22 02:11 mateocole

Typically GitHub Actions stuff is handled internally, but in this case I think it might be fine to make external, especially since there's a pretty clear implementation plan above ^

roryabraham avatar Nov 11 '22 16:11 roryabraham

Current assignee @mateocole is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] avatar Nov 11 '22 16:11 melvin-bot[bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @thesahindia (External)

melvin-bot[bot] avatar Nov 11 '22 16:11 melvin-bot[bot]

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

melvin-bot[bot] avatar Nov 11 '22 16:11 melvin-bot[bot]

It's weird that this assigned @iwiznia, since I was already assigned to the issue so should've been an eligible CME. Unassigning you @iwiznia!

Also going to remove the C+ review on this issue and PR. Because they can't really test the PR anyways, it's just not as valuable

roryabraham avatar Nov 11 '22 16:11 roryabraham

Job posted- https://www.upwork.com/ab/applicants/1591235610161598464/job-details

mateocole avatar Nov 12 '22 01:11 mateocole

Waiting for proposals

roryabraham avatar Nov 14 '22 21:11 roryabraham

Current assignee @mateocole is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] avatar Nov 17 '22 21:11 melvin-bot[bot]

Job added to Upwork: https://www.upwork.com/jobs/~0131b521e37dc4e067

melvin-bot[bot] avatar Nov 17 '22 21:11 melvin-bot[bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @mollfpr (External)

melvin-bot[bot] avatar Nov 17 '22 21:11 melvin-bot[bot]

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

melvin-bot[bot] avatar Nov 17 '22 21:11 melvin-bot[bot]

@cead22 taking this internal given the age of the issue as discussed here. Please try to move this forward with urgency so that we can get WAQ done!

@mollfpr sorry for the confusion. This should be Internal.

puneetlath avatar Nov 17 '22 21:11 puneetlath

@cead22, @mateocole Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] avatar Nov 21 '22 08:11 melvin-bot[bot]

Started a discussion about the solution here https://expensify.slack.com/archives/C03TQ48KC/p1669054325132319

cead22 avatar Nov 22 '22 00:11 cead22

This is fixed

roryabraham avatar Nov 28 '22 19:11 roryabraham