freeCodeCamp / classroom

BSD 3-Clause "New" or "Revised" License
143 stars 121 forks source link

Enable status checks in PRs #415

Closed lloydchang closed 1 year ago

lloydchang commented 1 year ago

Please enable:

  1. "Require status checks to pass before merging"
  2. "Require branches to be up to date before merging"

via GitHub Protected Branches

Describe the bug

  1. Some pull requests are merged that don't pass status checks
  2. Some pull requests are merged that aren't up to date

To Reproduce Steps to reproduce the behavior for 1. Some pull requests are merged that don't pass status checks

  1. Go to https://github.com/freeCodeCamp/classroom/pull/401
  2. Scroll down to the bottom of the page
  3. Click on "View Details" button
  4. It says:

    2 of 3 checks passed Build and test (16.14.2) Details

Expected behavior Pull requests are merged that don't pass status checks

Screenshots

Details

Screen Shot 2023-07-31 at 11 13 43 AM Screen Shot 2023-07-31 at 11 12 45 AM Screen Shot 2023-07-31 at 11 12 50 AM https://github.com/freeCodeCamp/classroom/actions ❌ [Modifying the data type being used for allChallenges in api_processor…](https://github.com/freeCodeCamp/classroom/actions/runs/5709645826) Classroom ci #283: Commit [a8de3b5](https://github.com/freeCodeCamp/classroom/commit/a8de3b57aa35ed79a097a020c3b98a912a00d6aa) pushed by [utsab](https://github.com/utsab) [main](https://github.com/freeCodeCamp/classroom) 16 hours ago 47s Screen Shot 2023-07-31 at 11 25 16 AM **Additional context** From https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches > [Repositories](https://docs.github.com/en/repositories) / [Branches and merges](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository) / [Manage protected branches](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches) > > **Managing protected branches** > > You can set up rules to protect certain branches in your repository. For example, you can block pull requests that don't pass status checks or require that pull requests have a specific number of approving reviews before they can be merged. > > Protected branches are available in public repositories with GitHub Free and GitHub Free for organizations, and in public and private repositories with GitHub Pro, GitHub Team, GitHub Enterprise Cloud, and GitHub Enterprise Server. For more information, see "[GitHub’s plans](https://docs.github.com/en/get-started/learning-about-github/githubs-plans)." Screen Shot 2023-07-31 at 11 15 13 AM **Reference:** https://stackoverflow.com/questions/58028682/github-branch-protection-require-status-checks-for-multiple-projects-in-a-singl ![l2q9s](https://github.com/freeCodeCamp/classroom/assets/1329685/5cecd5ae-6007-4ea6-acd2-5655407a548b) https://docs.wpvip.com/how-tos/required-status-checks/ Screen Shot 2023-07-31 at 11 23 05 AM


Thank you!

lloydchang commented 1 year ago

Relates to https://github.com/freeCodeCamp/classroom/issues/403

Enable GitHub's Merge Queue for freeCodeCamp classroom GitHub repository

GitHub's Merge Queue is in a similar space as GitHub Protected Branches but is a different feature.

utsab commented 1 year ago

HI @lloydchang, your recommendation makes sense. It doesn't look like I have the necessary permissions to enable the setting. We may need to ask @raisedadead to enable it.

raisedadead commented 1 year ago

Will Do @utsab - I'll add you as an admin too. Thanks.

The OP has too much information that is not warranted. Mind confirming the "check run" that you want to be enabled? Is it just the "build and test" workflow?

BTW, be mindful of the request:

1. Require status checks to pass before merging - This is OK 2. Require branches to be up to date before merging - This will create a lot of noise and repeated CI runs which can eat up the quota. I recommend against enabling it unless you have high-frequency async merges (members needing to validate their work in tandem with other members due to comms lag?). For context, even fCC proper doesn't need that.

@lloydchang quick protip :) - Keep titles short and issues succint - maintainers of large scale projects usually ask follow up questions if they need more info.

lloydchang commented 1 year ago

@raisedadead 2. We've had asynchronous pull request problems during Summer internships until August 19th

lloydchang commented 1 year ago

@raisedadead wrote:

maintainers of large scale projects…

Understood. You weren't the original audience when I reported, but I see your point.

raisedadead commented 1 year ago

We've had asynchronous pull request problems during Summer internships until August 19th

That seems more like a communication issue that is not ideal in distributed teams. I recommend using a channel (we have a discord) to coordinate peer reviews. We have battle-tested this approach with much of our work on the main repo by doing it in public.

Sure, "merge queues" and checks like "update branch" help - but from experience, they are more annoying in the real world.

My two cents. Happy contributing!

lloydchang commented 1 year ago

@raisedadead Thanks for your feedback re: communication issue

Confusion originated in June when fCC proper started a backwards-incompatible schema change, but didn't realize that it will break classroom.

fCC proper didn't coordinate it with classroom.

In July, that uncoordinated change from fCC proper was deployed, which caused confusion throughout classroom as functionality broke.

Meanwhile, two of the four teams assigned to classroom experienced asynchronous pull request problems.

My team (a fourth team) is a downstream consumer of the above.

It's been a matter of patience while the other teams resolve asynchronous issues and breakages that piled up without a queue.

IME, "merge queues" and checks like "update branch" help in the real world and are not annoying, but YMMV. :)

raisedadead commented 1 year ago

IME, "merge queues" and checks like "update branch" help in the real world and are not annoying, but YMMV. :)

Really? That's interesting.

I would love to look at some of the past projects where you have used the features. I am curious about what you did differently or the setting in which you collaborated. Feel free to share because they did get in the way of us over on the main repo.

Anyways, I'm just coming back to your other concern. We appreciate your eagerness to contribute; however, the inter-dependencies require a deeper understanding of our codebase/infrastructure.

Our current API has no guarantees, and changes may occur without prior notification - it's unfortunate, but it gets priority because it is live in the real world. If you find something broken, the quickest way is to contact us in the chat.

BTW, There is already a newer API in the works that should fit the needs of this project. The core maintainers of this project know all this; don't hesitate to contact them in our chat if you want to learn more.

Just to clarify, there is no set expectation or deadline for when the new API will be ready for use, even for this project.

The "Update branch" button has always been available when a PR is outdated. I'm not sure if you've been using it.

Details

image

I have already enabled the status check policy, as requested.

Unless one of the maintainers (👋🏽 I am one of them) requests, we will not enable the merge queue and or require updating PRs on every update to the main branch.

I would love to discuss this more, but I am limited on time and would want to keep the discussion to the core maintainers (with the burden of maintaining the repo). If you have further thoughts, please head over to the chat.

Thanks.