chore: PR template: only allow PR to be merged if its all green

[jennijuju]

Related Issues & Context

During lotus summit in June, 2023, it was reflected that the current lotus merge & release testing flow may have the following bug:

• A PR that introduces experimental features, small bug fixes, or updated deps may land in master(dev branch) without being fully tested. -Some Lotus contributors rely on the lotus release flow to cover the testing. However, that code change may land in a production stable release without full testing if the release steward didn't capture it in the release test plan.
• This may result in uncaptured regression, bugs, and untested features to be included in a stable release before shipping to users.

As a result, we proposed to start only "Hit the merge button when everything is 🟢 ", 🟢 means:

• CI is green.
• PR includes context and test plans
• lotus maintainers should review the proposed test plans as the PR is open & work with the author to align and finalize on the test plan.
• there is proper documentation for new & updated features/configurations
• A PR only gets to be merged if all above is 🟢

By doing so, we expect

• PR to land slower however with overall higher quality
• Reduce scope & risk during release testing flow & escalate the release testing process (as all PRs should have already been tested).

Proposed Changes

updated the PR template:

• added test plan session
• updated the changelog

Test Plan

N/A

Checklist

@lotus-maintainers will only approve the PR if all mandatory boxes below are checked:

• Commits have a clear commit message.
• PR title is in the form of of <PR type>: <area>: <change being made>
  • example: fix: mempool: Introduce a cache for valid signatures
  • PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
  • area, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps
• (optional) New or updated features/configuration have usage guidelines and/or documentation updates in Lotus Documentation.
• CI is green

[Stebalien]

Can we just use GitHub to enforce green CI? I think we've finally tackled most of our flaky tests.

[arajasek]

Sounds good to me!

[arajasek]

In practice, I feel like we don't do this, and are unlikely to start doing this. What do we think about updating this to say that the PR description itself should have documentation guidelines (we often, but not always do this today), and perhaps we should add such docs to the UNRELEASED section of the Changelog as needed?

[fridrik01]

I think I agree to this. We should at least require that PR includes the details about new feature/configuration and that preferably a test plan that demonstrates the usage. Updating UNRELEASED in the lotus should help capture the bigger items which can then collected and capture together in the lotus.filecoin.io docs.

[arajasek]

Can we just use GitHub to enforce green CI? I think we've finally tackled most of our flaky tests.

@Stebalien I think this is a "should also do" -- this PR seems more motivated about making sure that PRs include tests themselves, or descriptions of the manual testing that has been performed already, or AT LEAST description of the manual testing that we will have to do.

Definitely ALSO in favour of enforcing green CI now that we can -- happy to continue to be pulled into flaky builds to fix them.

[shrenujbansal]

*operational

[fridrik01]

I think I agree to this. We should at least require that PR includes the details about new feature/configuration and that preferably a test plan that demonstrates the usage. Updating UNRELEASED in the lotus should help capture the bigger items which can then collected and capture together in the lotus.filecoin.io docs.

[snissn]

Agree with what's stated here #10975 (comment)

1. I agree with this PR
2. I think we should set up GitHub to enforce green CI on PRs in a separate cake

[snadrus]

The plan needs a "force commit" option because blocking urgent fixes on flaky tests is the wrong business decision.

[snissn]

@snadrus brings up a very good point! Were we also talking about changing our branch model? I recall discussions about creating a more stable "next release" branch. Within the context of our branch model, we may want to expand the scope of explanations for when we can if we ever need to have a merge that is an override. We could possibly create a rule where there's a branch that the "emergency fixes" go into that does not need all tests to be green.

I'm curious though if we do indeed need that rule in practice. For the flaky tests, I've been finding it not a large burden to rerun tests until the pass in practice. Also people in this thread are chiming in that we have been improving flaky tests. Also, it concerns me that we are considering a scenario where we have an emergency fix that needs a branch merge yet despite this being an emergency fix we can't get our tests to pass. Are we talking about tests related to this feature or overall flaky tests?

I'm personally still inclined to confirm this PR and monitor to see if there are actually concerns with flaky tests or other emergencies and see if we need to later on modify our practice and rules to create an exception case for overriding the rule that all ci tests must past for a merge.

[github-actions]

All checks have completed

❌ Failed Check / Check (lint-all) (pull_request)
❌ Failed Test / Test (unit-cli) (pull_request)