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

Add comment when update protected branch fails #6706

Merged
merged 4 commits into from
Dec 14, 2021
Merged

Conversation

AndrewGable
Copy link
Contributor

@AndrewGable AndrewGable commented Dec 10, 2021

Details

Adds a comment when the deploy process fails for the mobile deployer to fix the error and merge the PR to continue the deploy process.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/185234

Tests

  1. Force this workflow to fail via these steps: Add comment when update protected branch fails #6706 (review)

@AndrewGable AndrewGable requested a review from a team as a code owner December 10, 2021 21:44
@AndrewGable AndrewGable self-assigned this Dec 10, 2021
@MelvinBot MelvinBot requested review from puneetlath and removed request for a team December 10, 2021 21:44
Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically I think we can force updateProtectedBranch to fail by:

  1. With the deploy checklist locked, merge and CP a pull request with some change in a specific file.
  2. Merge and CP another PR changing that same file.
  3. This should cause a merge conflict on staging when you go to CP the second PR to staging, which would fail the updateProtectedBranch workflow.

.github/workflows/updateProtectedBranch.yml Outdated Show resolved Hide resolved
.github/workflows/updateProtectedBranch.yml Outdated Show resolved Hide resolved
.github/workflows/updateProtectedBranch.yml Outdated Show resolved Hide resolved
AndrewGable and others added 3 commits December 10, 2021 17:58
Co-authored-by: Rory Abraham <47436092+roryabraham@users.noreply.github.com>
Co-authored-by: Rory Abraham <47436092+roryabraham@users.noreply.github.com>
Co-authored-by: Rory Abraham <47436092+roryabraham@users.noreply.github.com>
@AndrewGable
Copy link
Contributor Author

Updated! I'll test once merged, thank you for the help coming up with the test steps.

Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM. I think I might've been wrong about the test steps though – even if we have a merge conflict, I think we'll just commit the <== HEAD diff stuff, and then theoretically the PR would be mergeable. So actually I'm not sure how we reproduce an unmergable PR 🤷

@AndrewGable
Copy link
Contributor Author

@puneetlath did you want to review this one?

@puneetlath
Copy link
Contributor

I don't have a lot of context or knowledge in this area, but I took a look and it seems to make sense. Thanks for giving me the opportunity!

@puneetlath puneetlath merged commit 6d32316 into main Dec 14, 2021
@puneetlath puneetlath deleted the andrew-merge-fail branch December 14, 2021 16:29
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @puneetlath in version: 1.1.20-3 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @Julesssss in version: 1.1.21-1 🚀

platform result
🤖 android 🤖 failure ❌
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants