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

Skip most PR checks on automated pull requests #3499

Merged
merged 1 commit into from
Jun 22, 2021

Conversation

roryabraham
Copy link
Contributor

@roryabraham roryabraham commented Jun 9, 2021

Details

Just a small optimization for GH Actions to skip checks on automated pull requests. This is not super important, but has at least three potential benefits:

  1. Less noise in the GHA console.
  2. Less likely to run into rate-limits or throttling with the number of concurrent workflow runs.
  3. Generally less load on GH servers hopefully leads to faster actions in the aggregate.

Also cleans up indentation on these files to be two spaces, which we've standardized on in our other .yml workflows.

Fixed Issues

n/a

Tests

The checks on this PR should run. When merged, the checks on the version-bump PR should be skipped. Create another PR, and the PR checks should run.

QA Steps

None

Tested On

GitHub only

@roryabraham roryabraham requested a review from AndrewGable June 9, 2021 22:29
@roryabraham roryabraham self-assigned this Jun 9, 2021
@roryabraham roryabraham requested a review from a team as a code owner June 9, 2021 22:29
@MelvinBot MelvinBot requested review from Beamanator and removed request for a team June 9, 2021 22:29
@AndrewGable
Copy link
Contributor

I'm trying to think of any unintended side effects of this.. If we introduce a lint error, but still merge, do we stop deploys from happening today? And after this PR we might ship code that hasn't been tested/linted?

@Beamanator
Copy link
Contributor

I was wondering what kinds of things OSBotify does, and from looking around it seems like it can do:

  1. Create CP PRs (with commits) - example: 🍒 Cherry pick PR #3486 to staging 🍒 #3495
  2. Update Version PRs - example: Update version to 1.0.65-6 on main #3464
  3. Create Deploy Checklist issues - example: Deploy Checklist: Expensify.cash 2021-06-01 #3279

Of these 3, it seems like only the first would require linting & tests to still be run, right? Would this change prevent all of the GHAs from running on CPs?

@roryabraham
Copy link
Contributor Author

roryabraham commented Jun 11, 2021

So the bottom line is that the only code OSBotify will write, merge, and deploy himself is the version bump PRs (example: #3464). And those are merge within seconds, so there's no point in running tests or lints because we don't ever wait for them to finish anyways.

During CP's, he will cherry pick commits that have already been tested, linted, approved by a human, and merged to main. If there is a merge conflict, he will create the PR but not merge it. Then a human will have to take over, and as soon as they push an update to the pull request, the github.actor will no longer be OSBotify, and the tests & linter will run. In the case when a CP PR is auto-merged, we aren't waiting for tests and linting to finish anyways, so the results of these tests are just ignored.

For the creation of deploy checklists, I don't think that's really related to any of the things we're disabling here: iOS E2E tests, Jest Unit tests, CLA, GHA bundle & podfile validation. All of these are checks that run on PRs, not issues. Except CLA I guess, which we don't really need on deploy checklists anyways.

Copy link
Contributor

@Beamanator Beamanator left a comment

Choose a reason for hiding this comment

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

Thanks for the responses and clarifications @roryabraham ! I'm a 👍

@roryabraham
Copy link
Contributor Author

What about you @AndrewGable ? 👍 or 👎

@AndrewGable
Copy link
Contributor

Your explanation sounds good, but could we beed up the tests?

@roryabraham
Copy link
Contributor Author

could we beed up the tests

I'm assuming you meant speed up the tests, and yeah ... there's at least one issue to do that.

@AndrewGable
Copy link
Contributor

AndrewGable commented Jun 19, 2021 via email

@roryabraham
Copy link
Contributor Author

Added this for after the PR is merged:

Create another PR, and the PR checks should run.

Other than that, I'm open to suggestions for other tests we should add.

@AndrewGable AndrewGable merged commit 75671ce into main Jun 22, 2021
@AndrewGable AndrewGable deleted the Rory-SkipTestOnVersionBumps branch June 22, 2021 14:23
@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 in version: 1.0.73-4🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.74-0🚀

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