Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[$250] Race condition with the CP Staging label #7284

Closed
roryabraham opened this issue Jan 17, 2022 · 29 comments
Closed

[$250] Race condition with the CP Staging label #7284

roryabraham opened this issue Jan 17, 2022 · 29 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review

Comments

@roryabraham
Copy link
Contributor

roryabraham commented Jan 17, 2022

Coming from #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:


      ⚠️ Heads up! ⚠️ 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:


      ⚠️ Heads up! ⚠️ 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.


Upwork Automation - Do Not Edit

@roryabraham
Copy link
Contributor Author

No update here yet.

@roryabraham
Copy link
Contributor Author

This is pretty low priority, no update.

@MelvinBot MelvinBot removed the Overdue label Mar 23, 2022
@melvin-bot melvin-bot bot added the Overdue label Apr 25, 2022
@roryabraham
Copy link
Contributor Author

Again no update.

@melvin-bot melvin-bot bot removed the Overdue label Apr 25, 2022
@melvin-bot melvin-bot bot added the Overdue label May 26, 2022
@roryabraham
Copy link
Contributor Author

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

@melvin-bot melvin-bot bot removed the Overdue label Jun 1, 2022
@melvin-bot melvin-bot bot added the Overdue label Jul 4, 2022
@roryabraham
Copy link
Contributor Author

Not a priority

@melvin-bot melvin-bot bot removed the Overdue label Jul 7, 2022
@melvin-bot melvin-bot bot added the Overdue label Aug 8, 2022
@roryabraham
Copy link
Contributor Author

No update

@melvin-bot melvin-bot bot removed the Overdue label Aug 8, 2022
@melvin-bot melvin-bot bot added the Overdue label Sep 9, 2022
@roryabraham
Copy link
Contributor Author

No update

@melvin-bot melvin-bot bot removed the Overdue label Sep 29, 2022
@puneetlath puneetlath added the Bug Something is broken. Auto assigns a BugZero manager. label Oct 19, 2022
@melvin-bot melvin-bot bot added the Overdue label Oct 31, 2022
@roryabraham
Copy link
Contributor Author

No update

@melvin-bot melvin-bot bot removed the Overdue label Oct 31, 2022
@puneetlath puneetlath removed External Added to denote the issue can be worked on by a contributor Engineering Help Wanted Apply this label when an issue is open to proposals by contributors labels Nov 17, 2022
@melvin-bot melvin-bot bot removed the Overdue label Nov 17, 2022
@puneetlath puneetlath added Engineering Internal Requires API changes or must be handled by Expensify staff Overdue External Added to denote the issue can be worked on by a contributor and removed Engineering labels Nov 17, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 17, 2022

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

@melvin-bot
Copy link

melvin-bot bot commented Nov 17, 2022

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

@melvin-bot
Copy link

melvin-bot bot commented Nov 17, 2022

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

@melvin-bot
Copy link

melvin-bot bot commented Nov 17, 2022

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

@melvin-bot melvin-bot bot removed the Overdue label Nov 17, 2022
@puneetlath puneetlath added Overdue and removed External Added to denote the issue can be worked on by a contributor labels Nov 17, 2022
@puneetlath
Copy link
Contributor

@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.

@melvin-bot
Copy link

melvin-bot bot commented Nov 21, 2022

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

@cead22
Copy link
Contributor

cead22 commented Nov 22, 2022

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

@melvin-bot melvin-bot bot removed the Overdue label Nov 22, 2022
@cead22 cead22 mentioned this issue Nov 22, 2022
96 tasks
@cead22 cead22 added the Reviewing Has a PR in review label Nov 22, 2022
@roryabraham
Copy link
Contributor Author

This is fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

9 participants