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

Fix part of #3926: Add support for CI checks in merge queues [Blocked: #4929] #4913

Closed
wants to merge 1 commit into from

Conversation

BenHenning
Copy link
Sponsor Member

@BenHenning BenHenning commented Mar 21, 2023

Explanation

Fixes part of #3926 by introducing the necessary workflow adjustments to ensure all required CI workflows will be run when a PR is added to a merge queue (per https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#merge_group).

Note that this PR needs to be merged before merge queues can actually be enabled.

#3926 has some context, but the main benefit that we'll see from merge queues is the ability to avoid needing to keep branches up-to-date with the latest develop without risking non-conflicting changes causing breakages/incompatibilities (since merge queues always verify against the latest changes relative to that PR, even if others go in before it). They technically also offer better merge throughput, but this won't really affect us since we generally only have a few PRs merged each week (versus every hour, for example).

Essential Checklist

  • The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

N/A -- This is a GitHub infrastructure-only change and won't affect build outputs (including the app).

@BenHenning BenHenning changed the title Fix #3926: Add support for CI checks in merge queues Fix part of #3926: Add support for CI checks in merge queues Mar 21, 2023
@BenHenning BenHenning marked this pull request as ready for review March 21, 2023 00:36
@BenHenning
Copy link
Sponsor Member Author

@adhiamboperes and/or @seanlip PTAL & let me know if you have any questions. I plan to enable merge queues once this PR goes in.

@BenHenning BenHenning enabled auto-merge (squash) March 21, 2023 00:37
@BenHenning BenHenning enabled auto-merge (squash) March 21, 2023 00:39
@BenHenning BenHenning marked this pull request as draft March 21, 2023 00:54
auto-merge was automatically disabled March 21, 2023 00:54

Pull request was converted to draft

@BenHenning
Copy link
Sponsor Member Author

This actually needs more work. I realized that the compute_affected_tests workflow needs to always run all tests when a PR is in the merge queue (vs. just the diff that we run on regular CI pushes) to handle the rare case where it misses a target (since changes to develop itself always run the full test suite). Otherwise, the queue will likely devolve to almost running all tests, anyway, if there are stacked changes (since it'll be diffing against develop & automatically including pending changes earlier in the queue).

@BenHenning BenHenning self-assigned this Mar 21, 2023
@seanlip
Copy link
Member

seanlip commented Mar 21, 2023

Just a note, I think Web had some issues with enabling merge queue due to the oppiabot CLA check. Adding @U8NWXD @gp201 in case they can shed more light on that.

@seanlip seanlip assigned BenHenning, U8NWXD and gp201 and unassigned BenHenning Mar 21, 2023
@gp201
Copy link
Member

gp201 commented Mar 21, 2023

This PR can be merged. But please do NOT enable merge-queue. This is because oppiabot is a required check in the repositories, and oppiabot does not execute the CI check since it is not a push event. I'm currently working on a fix for that this week. When the fix is ready, I'll make a PR to both the repositories and request for the merge queue feature to be enabled.

@U8NWXD U8NWXD removed their assignment Mar 22, 2023
@BenHenning
Copy link
Sponsor Member Author

This PR can be merged. But please do NOT enable merge-queue. This is because oppiabot is a required check in the repositories, and oppiabot does not execute the CI check since it is not a push event. I'm currently working on a fix for that this week. When the fix is ready, I'll make a PR to both the repositories and request for the merge queue feature to be enabled.

Ah, thanks for letting me know @gp201. I find it a bit odd that GitHub uses different configurations for pull request & merge queue kick-offs but uses the same branch required checks list. Either way, I probably won't get to this until late next week at the earliest, so I'll keep an eye out for an Oppiabot update.

@gp201
Copy link
Member

gp201 commented Mar 29, 2023

@BenHenning merge-queue has been enabled for the oppia repository. As long as all the required checks have the merge_group trigger, merge-queue should work without any issue.

@gp201 gp201 removed their assignment Mar 29, 2023
@BenHenning
Copy link
Sponsor Member Author

Thanks @gp201!

For Android, #4929 is the main blocker for this PR as it updates our test computation process to be more flexible in the base branch it uses (which is necessary for merge queues). I still need to think through the integration part a bit more as I'm not entirely sure yet how we should be deciding tests to run.

@BenHenning BenHenning changed the title Fix part of #3926: Add support for CI checks in merge queues Fix part of #3926: Add support for CI checks in merge queues [Blocked: #4929] Apr 4, 2023
@oppiabot
Copy link

oppiabot bot commented Apr 11, 2023

Hi @BenHenning, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Apr 11, 2023
@oppiabot oppiabot bot closed this Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Corresponds to items that haven't seen a recent update and may be automatically closed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants