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

Assign CP PRs to the deployer that triggered them #4602

Merged
merged 7 commits into from
Sep 3, 2021

Conversation

HorusGoul
Copy link
Contributor

@HorusGoul HorusGoul commented Aug 12, 2021

Details

Adds a new step in the cherry-pick job to assign the deployer to the PR with conflicts that need to be fixed manually.

Fixed Issues

$ #4036

Tests/QA Steps

  1. Create a PR with changes in a given file.
  2. Give the PR the CP Staging label.
  3. Merge the PR.
  4. Create another PR with changes in the same file (to ensure that there will be conflicts on staging).
  5. Give the PR the CP Staging label.
  6. Merge the PR.
  7. The CP PR created for the second PR will be created but not automerged, due to conflicts.
  8. The CP PR should be auto-assigned to the author of the original (second) PR.

@HorusGoul HorusGoul self-assigned this Aug 12, 2021
@HorusGoul HorusGoul requested a review from roryabraham August 12, 2021 11:55
@HorusGoul HorusGoul marked this pull request as ready for review August 12, 2021 11:55
@HorusGoul HorusGoul requested a review from a team as a code owner August 12, 2021 11:55
@MelvinBot MelvinBot requested review from pecanoro and removed request for a team August 12, 2021 11:56
@pecanoro
Copy link
Contributor

Ohh this is going to be really hard to QA since we have to actually modify files. Do you have any suggestions of what files we could modify? Maybe a README file or something? Should we simply hope for the best and see if it works when this happens? Also, I just discovered we have a CP label haha

pecanoro
pecanoro previously approved these changes Aug 12, 2021
.github/workflows/cherryPick.yml Outdated Show resolved Hide resolved
@HorusGoul
Copy link
Contributor Author

@pecanoro Yes! I'd also say changing a README file is the easiest way to test this.

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.

Couple things here:

  1. In order to add or adjust the inputs or outputs of a GH action, we need to also adjust the action metadata file
  2. Now that this action it doing more than getting the merge commit for a pull request, we should rename it to something more generic, perhaps just getPullRequestDetails
  3. I do wonder if there's an existing community action to just get all the details for an arbitrary pull request. If so, we could maybe use that and get rid of this custom action altogether.

@roryabraham
Copy link
Contributor

The only community action I found is this one, which doesn't look like it has all the outputs we need. So there are two choices:

Continue with our own custom action, include only the outputs we need, and rename it do something more generic like getPullRequestDetails

OR

  1. Fork that community action to add outputs for:
    1. merge_commit_sha
    2. merged_by
  2. Point our workflow at the forked action
  3. Create a PR to update the community action. Once that PR is merged, point our workflow back at the upstream.

@roryabraham
Copy link
Contributor

Up to you which you prefer!

@HorusGoul
Copy link
Contributor Author

I'm going to go with the first option, as we have this custom logic related to the OSBotify actor, and we would still need a custom action for that.

@HorusGoul HorusGoul requested a review from roryabraham August 17, 2021 15:57
@pecanoro
Copy link
Contributor

pecanoro commented Sep 1, 2021

Is it ready for another review? I see you updated the PR

HorusGoul and others added 2 commits September 2, 2021 12:50
Co-authored-by: Rory Abraham <47436092+roryabraham@users.noreply.github.com>
@roryabraham roryabraham requested a review from pecanoro September 2, 2021 17:50
@roryabraham
Copy link
Contributor

All yours @pecanoro

@pecanoro pecanoro merged commit 9d06504 into main Sep 3, 2021
@pecanoro pecanoro deleted the horus-assign-pr-to-deployer branch September 3, 2021 11:00
@OSBotify
Copy link
Contributor

OSBotify commented Sep 3, 2021

🚀 Deployed to staging by @pecanoro in version: 1.0.92-6 🚀

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

@roryabraham
Copy link
Contributor

@HorusGoul this is going to need internal QA – we know that it didn't break the CP flow (because we ran a CP this morning), but we still need to verify that it works as expected. Not going to block the deploy on this because there's no good reason to - it doesn't affect any of the app code.

@OSBotify
Copy link
Contributor

OSBotify commented Sep 4, 2021

🚀 Deployed to production by @roryabraham in version: 1.0.93-1 🚀

platform result
🤖 android 🤖 success ✅
🖥 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