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

Enable status checks in PRs #415

Closed
lloydchang opened this issue Jul 31, 2023 · 8 comments
Closed

Enable status checks in PRs #415

lloydchang opened this issue Jul 31, 2023 · 8 comments

Comments

@lloydchang
Copy link
Contributor

lloydchang commented Jul 31, 2023

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 Modifying the data type being used for allChallenges in api_processor… #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…
Classroom ci #283: Commit a8de3b5 pushed by utsab
main
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 / Branches and merges / Manage 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."

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://docs.wpvip.com/how-tos/required-status-checks/

Screen Shot 2023-07-31 at 11 23 05 AM


Thank you!

@lloydchang
Copy link
Contributor Author

Relates to #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
Copy link
Collaborator

utsab commented Aug 1, 2023

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
Copy link
Member

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.

@raisedadead raisedadead changed the title Please enable 1. "Require status checks to pass before merging" and 2. "Require branches to be up to date before merging" via GitHub Protected Branches Enable status checks in PRs Aug 1, 2023
@lloydchang
Copy link
Contributor Author

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

@lloydchang
Copy link
Contributor Author

@raisedadead wrote:

maintainers of large scale projects…

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

@raisedadead
Copy link
Member

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
Copy link
Contributor Author

lloydchang commented Aug 1, 2023

@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
Copy link
Member

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.

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

No branches or pull requests

3 participants