From d4521da82c64fb6c60a55decc194c131559bd2b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Langa?= Date: Fri, 3 Nov 2023 17:00:40 +0100 Subject: [PATCH 1/3] Add section on keeping CI green --- getting-started/git-boot-camp.rst | 8 ++-- getting-started/pull-request-lifecycle.rst | 50 ++++++++++++++++++++-- triage/triaging.rst | 1 + 3 files changed, 52 insertions(+), 7 deletions(-) diff --git a/getting-started/git-boot-camp.rst b/getting-started/git-boot-camp.rst index 9b8981eb5..6b30ef08b 100644 --- a/getting-started/git-boot-camp.rst +++ b/getting-started/git-boot-camp.rst @@ -465,9 +465,11 @@ You can read more about what to look for before accepting a change :ref:`here `. All pull requests have required checks that need to pass before a change -can be merged. At any point, a core developer can schedule an automatic merge -of the change by -clicking the gray ``Enable auto-merge (squash)`` button. You will find +can be merged. See :ref:`"Keeping CI green" ` for some +simple things you can do to help the checks turn green. + +At any point, a core developer can schedule an automatic merge of the change +by clicking the gray ``Enable auto-merge (squash)`` button. You will find it at the bottom of the pull request page. The auto-merge will only happen if all the required checks pass, but the PR does not need to have been approved for a successful auto-merge to take place. diff --git a/getting-started/pull-request-lifecycle.rst b/getting-started/pull-request-lifecycle.rst index e2dde81f6..d24596703 100644 --- a/getting-started/pull-request-lifecycle.rst +++ b/getting-started/pull-request-lifecycle.rst @@ -41,8 +41,8 @@ Here is a quick overview of how you can contribute to CPython: #. `Create Pull Request`_ on GitHub to merge a branch from your fork -#. Make sure the continuous integration checks on your Pull Request - are green (i.e. successful) +#. Make sure the :ref:`continuous integration checks on your Pull Request + are green ` (i.e. successful) #. Review and address `comments on your Pull Request`_ @@ -472,7 +472,7 @@ code and leave comments in the pull request or issue tracker. 'merge-ready', you should always make sure the entire test suite passes. Leaving a pull request review on GitHub -======================================= +--------------------------------------- When you review a pull request, you should provide additional details and context of your review process. @@ -487,14 +487,55 @@ Instead of simply "approving" the pull request, leave comments. For example: #. Comment on what is "good" about the pull request, not just the "bad". Doing so will make it easier for the PR author to find the good in your comments. +#. Look at any failures in CI on the current PR. See :ref:`"Keeping CI green" + ` below for simple things you can do to help move the PR forward. + Dismissing review from another core developer -============================================= +--------------------------------------------- A core developer can dismiss another core developer's review if they confirmed that the requested changes have been made. When a core developer has assigned the PR to themselves, then it is a sign that they are actively looking after the PR, and their review should not be dismissed. +.. _keeping-ci-green: + +Keeping Continuous Integration green +==================================== + +Our change management workflows generally won't allow merging PRs with +failures on them. Therefore, if you see a CI failure on a PR, have a look +what it is about. + +Usually the failure will be directly related to the changes in the current +PR. If you happen to have any insight into the failure, let the author know +in a review comment. CI runs sometimes generate thousands of lines of output. +Even something as simple as finding the traceback and putting it in the +comment will be helpful to the PR author. + +If the failure doesn't look related to the change you're looking at, check +if it's not present on the `Release Status`_ Buildbot dashboard as well. +If so, that means the failure was introduced in a prior change. Using Buildbot's +UI you can find which PR introduced the issue and comment there that it +affects other PRs. + +If you still don't see where the failure originates from, check for +an "This branch is out-of-date with the base branch" sign next to the +list of executed checks. Clicking "Update branch" next to this message +will merge in the latest changes from the base branch into the PR. + +If this still doesn't help with the failure on the PR, you can try +to re-run that particular failed check. Go to the red GitHub Action job, +and find a button on the top right called "Re-run jobs". Select +"Re-run failed jobs". The button will only be present when all other jobs +finished running. + +Re-running failed jobs shouldn't be your first instinct but it is occasionally +helpful because distributed systems can have intermittent failures, and +some of our unit tests are sensitive to overloaded virtual machines. +If you identify such flaky behavior, look for an issue in the `issue tracker`_ +that describes this particular flakiness. Create a new issue if you can't +find one. Committing/rejecting ==================== @@ -522,3 +563,4 @@ accepts your pull request. .. _issue tracker: https://github.com/python/cpython/issues .. _Core Development Discourse category: https://discuss.python.org/c/core-dev/23 +.. _Release Status: https://buildbot.python.org/all/#/release_status \ No newline at end of file diff --git a/triage/triaging.rst b/triage/triaging.rst index 89919450d..1d9e8809f 100644 --- a/triage/triaging.rst +++ b/triage/triaging.rst @@ -94,6 +94,7 @@ you can help by making sure the pull request: * includes a :ref:`NEWS entry ` (if needed) * includes the author in ``Misc/ACKS``, either already or the patch adds them * doesn't have conflicts with the ``main`` branch +* :ref:`doesn't have failing CI checks ` Doing all of this allows core developers and :ref:`triagers ` to more quickly look for subtle issues that only people with extensive From 35aa19808c4dadb62b68024954070b26f6c352fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Langa?= Date: Fri, 3 Nov 2023 17:06:29 +0100 Subject: [PATCH 2/3] Run pre-commit --- getting-started/pull-request-lifecycle.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/getting-started/pull-request-lifecycle.rst b/getting-started/pull-request-lifecycle.rst index d24596703..48ca2ee80 100644 --- a/getting-started/pull-request-lifecycle.rst +++ b/getting-started/pull-request-lifecycle.rst @@ -563,4 +563,4 @@ accepts your pull request. .. _issue tracker: https://github.com/python/cpython/issues .. _Core Development Discourse category: https://discuss.python.org/c/core-dev/23 -.. _Release Status: https://buildbot.python.org/all/#/release_status \ No newline at end of file +.. _Release Status: https://buildbot.python.org/all/#/release_status From a738be9e84add7ead42341b8ebb543df3c2ed33a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Langa?= Date: Tue, 6 Feb 2024 21:04:00 +0100 Subject: [PATCH 3/3] Apply suggestions from code review Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com> Co-authored-by: Jelle Zijlstra Co-authored-by: Ezio Melotti --- getting-started/pull-request-lifecycle.rst | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/getting-started/pull-request-lifecycle.rst b/getting-started/pull-request-lifecycle.rst index 48ca2ee80..1c16f1592 100644 --- a/getting-started/pull-request-lifecycle.rst +++ b/getting-started/pull-request-lifecycle.rst @@ -500,11 +500,11 @@ the PR, and their review should not be dismissed. .. _keeping-ci-green: -Keeping Continuous Integration green +Keeping continuous integration green ==================================== Our change management workflows generally won't allow merging PRs with -failures on them. Therefore, if you see a CI failure on a PR, have a look +failures. Therefore, if you see a CI failure on a PR, have a look what it is about. Usually the failure will be directly related to the changes in the current @@ -516,17 +516,17 @@ comment will be helpful to the PR author. If the failure doesn't look related to the change you're looking at, check if it's not present on the `Release Status`_ Buildbot dashboard as well. If so, that means the failure was introduced in a prior change. Using Buildbot's -UI you can find which PR introduced the issue and comment there that it +UI you can find which PR introduced the issue and comment that it affects other PRs. If you still don't see where the failure originates from, check for -an "This branch is out-of-date with the base branch" sign next to the +a "This branch is out-of-date with the base branch" sign next to the list of executed checks. Clicking "Update branch" next to this message will merge in the latest changes from the base branch into the PR. If this still doesn't help with the failure on the PR, you can try to re-run that particular failed check. Go to the red GitHub Action job, -and find a button on the top right called "Re-run jobs". Select +click on the "Re-run jobs" button on the top right, and select "Re-run failed jobs". The button will only be present when all other jobs finished running.