diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1b1753c..1f788a6 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -639,14 +639,13 @@ to verify the code works as expected. If PRs are merged with failing status checks, there are potential bugs in the code which may lead to downtime or other operational issues. -PRs can only be merged if all status checks pass. Some exceptions could -include: - +PRs should only be merged if all status checks pass. Changes should only be landed +if are urgently needed in production and the failures can't be fixed in a reasonable +timeline. Some examples include: * If the team has recently taken over a new charm and changes are urgently - needed and the test suit of the existing charm is failing for spurious - reasons. +needed and the test suit of the existing charm is failing for spurious reasons. * A tool we depend on is not working, no previous working version is available - and a change is urgently needed to fix a critical issue in production. +and a change is urgently needed to fix a critical issue in production. Even in the above cases it is not clear whether it is reasonable to merge a PR with failing checks due to the high risks this entails. @@ -654,14 +653,16 @@ Judgement is required in these cases weighing the risks of introducing bugs with the urgency and impact of the underlying need to land the change. -Alternatives to merging the PR with failing status checks include: +Before merging the PR with failing status checks evaluate the following options: * Change the code to fix the problem. +* Re-run the test in case of suspected flakiness. +* Wait for an upstream fix for the issue. +* Remove the test if it doesn't add value. * Disable the status check (e.g., mark the test as [`xfail`](https://docs.pytest.org/en/7.1.x/how-to/skipping.html)). This should not be done lightly as the value the status check provides to the team - is lost. -* Wait for an upstream fix for the issue. + is lost and tracking a disabled test is challenging. If it is deemed that a change should land despite a failing status check, the following artifacts should be added to the PR: @@ -673,9 +674,9 @@ following artifacts should be added to the PR: (e.g., because the fix is needed in production and the status check is failing due to problems with GitHub). -One of the repository admins should then be asked to review the changes and -merge the PR after completing the review. The PR should be otherwise ready to be -merged (e.g., has been approved). +The PR should be otherwise ready to be merged (e.g., has been approved). + +Note that only repository admins will be able to force merge pull requests. Resolving the underlying reason the status check is failing should be high priority so that the team can rely on the automation again.