-
Notifications
You must be signed in to change notification settings - Fork 412
Restructure CI with tiered PR checks and release gating #6241
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
base: main
Are you sure you want to change the base?
Conversation
3b63721 to
d57584b
Compare
d57584b to
6fbfe51
Compare
Move release-checks, hold, and tag-release-branch into the ci workflow (formerly all-tests) so the release hold requires both release-checks and all-tests-passed. Remove standalone create-tag workflow.
The "ci" workflow already includes everything
2758b9b to
4d78abd
Compare
…-comments` is triggered" This reverts commit f6a32b2.
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
tonidero
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great! mostly some naming and small thing comments 🙌
.circleci/config.yml
Outdated
| steps: | ||
| - run: | ||
| name: Full suite gate passed | ||
| command: echo "Tier 1 passed and full tests approved - unlocking Tier 2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the Tier 1 and Tier 2 comment can be confusing down the line once we forget or for new people. Maybe it should be something a bit more descriptive like Reduced Test Suite and Full Test Suite?
This applies to all mentions of these in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion! I like it!
.circleci/config.yml
Outdated
| - release-checks | ||
| - tag-release-branch: | ||
| - approve-full-tests | ||
| - lint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm so seems we need to wait for the "reduced test suite" to finish before continuing with the full suite by having these here if I understand correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct! I made it so that we can approve the "approve-full-tests" immediately, but yes, the full test suite will wait for the reduced test suit to complete before starting running.
WDYT? Would you rather have it run in parallel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I feel if you accept the hold job before it finishes the others, it makes sense to run in parallel. That’s my 2 cents though, NABD :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. Tbh, I also had doubts about this.
it makes sense to run in parallel
That's actually how it still works with @RCGitBot please test: everything runs in parallel. The advantage of the hold job would be that it doesn't re-run the whole CI workflow, but just "unlocks" the full test suite.
So I'll remove the requirement of the other jobs for the full suite to start running. We can always tweak it in the future. Thanks for the suggestion!
.circleci/config.yml
Outdated
| # Summary gate: blocks merge until all Tier 2 jobs pass | ||
| # Add "ci/circleci: all-tests-passed" as a required check in GitHub | ||
| # ============================================================= | ||
| - all-tests-passed: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should rename to all-tasks-finished instead? So it's something we can reuse for the release workflow as well?
.circleci/config.yml
Outdated
| # - Name: action | ||
| # - Value: run-manual-tests | ||
| all-tests: | ||
| ci: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I'm not fully in love with this name for this workflow... Should it be release_or_main?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. I was torn on this one, as it's a workflow serving two purposes... it's true that it only runs on release branches or main. Honestly, I don't have a strong opinion and I do think release-or-main is clearer than just ci. however, this is the workflow that will run when we trigger the pipeline with the run-manual-tests parameter from CircleCI or with @RCGitBot please test.
Running tests manually won't be useful anymore for PRs, as now we'll have the hold job. However, there are some scenarios where we may want to run the release-or-main workflow for main (e.g. sometimes it does not get triggered correctly after merging a PR, so it needs to be run manually).
Still, having it be called release-or-main is clearer I think 👍
.circleci/config.yml
Outdated
| # ============================================================= | ||
| # Summary gate: all tests passed | ||
| # ============================================================= | ||
| - all-tests-passed: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would allow a release PR to be merged without approving the hold job... Maybe this should run only after the tag_current_branch job has run successfully?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm that's a good catch. But then, AFAIK, I don't think we can achieve this.
If I'm not wrong, GitHub status checks cannot be made depend on the source branch. I.e. they are defined for all PRs with main as the base branch. That means the check we decide to have must apply to all PRs, all-tasks-finished in this case.
Now, on CI config.yml, this would mean that all-tasks-finished should be different for release branches. I.e. we would need all-tasks-finished to require tag-release-branch. But that would not work for non-release PRs into main, where tag-release-branch is never run.
Am I missing something perhaps? I can try to think on workarounds for this though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was confused, as all-tasks-passed (formerly, briefly known as all-tasks-finished) can actually have different requires in release-or-main vs run-all-tests.
So I implemented your proposal in fcb9287.
It makes total sense and it's way better to block merging the Release PR until after tag-release-branch runs. Great great suggestions! Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After some internal discussion, I've reverted it back to what we had before.
The reasoning is sometimes we need to push changes to the release branch after the release happens (main example of this: merging main into the release branch).
This way, we can still merge release PRs into main even after new commits pushed to it.
In any case, with this PR + #6243 the whole release process should improve, as we should not manually merge release PRs anymore.
We can always iterate
…release-branch`" This reverts commit fcb9287.
Co-authored-by: Cursor <[email protected]>
|
@RevenueCat/coresdk I think this is now ready to be reviewed. I believe I addressed all comments from @tonidero's great review. |
Motivation
Currently only a subset of tests run on PRs, and the full suite (
all-tests) only runs onmain/releaseor via manual trigger. New contributors don't know about the manual trigger, so PRs can be merged without the full test suite passing.Additionally, on release branches the
holdjob for tagging a release only requiresrelease-checksto pass — it does not require the full test suite. This means a release can be approved and tagged even if some tests are failing.Description
run-all-testsworkflow for PR branches only (formerlybuild-testworkflow): splits jobs into two groups with a hold-gated approval step.lint,check-api-changes,validate-package-swift,run-test-ios-26,pod-lib-lint,run-revenuecat-ui-ios-26,emerge_purchases_ui_snapshot_tests,emerge_binary_size_analysis,build-tv-watch-and-macos,build-visionosapprove-full-tests.all-tasks-passed: summary gate requiring all jobs to pass — set as a required GitHub check to block merges.mainandrelease/*branches to avoid redundant runs.release-or-mainworkflow (renamed fromall-tests): runs the full test suite onmain,release/*, and manual triggers.all-tasks-passedsummary gate requiring all jobs.release-checks,hold, andtag-release-branchfrom the removedcreate-tagworkflow, so the release hold jobapprove-releaserequires bothrelease-checksandall-tasks-passed.approve-full-teststo unlock Full Test Suite. Merge blocked untilall-tasks-passed.approve-releaserequires bothrelease-checksandall-tasks-passed.Note
Triggering the full test suite via
@RCGitBot please testis not needed anymore for PRs intomain, sincebuild-testalready executes and requires the full suite (gated behindapprove-full-tests). This trigger can still be useful for PRs that have a base branch different tomain. In addition, therun-manual-testsCI parameter can still be useful for WIP branches that don't have an associated PR.