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

Add section on keeping CI green #1215

Merged
merged 3 commits into from
Feb 6, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions getting-started/git-boot-camp.rst
Original file line number Diff line number Diff line change
Expand Up @@ -465,9 +465,11 @@ You can read more about what to look for before accepting a change
:ref:`here <committing>`.

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" <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.
Expand Down
50 changes: 46 additions & 4 deletions getting-started/pull-request-lifecycle.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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 <keeping-ci-green>` (i.e. successful)

#. Review and address `comments on your Pull Request`_

Expand Down Expand Up @@ -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.
Expand All @@ -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"
<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
ambv marked this conversation as resolved.
Show resolved Hide resolved
====================================

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
ambv marked this conversation as resolved.
Show resolved Hide resolved
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.
Comment on lines +511 to +512
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They might also be able to fix them themselves.

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
ambv marked this conversation as resolved.
Show resolved Hide resolved
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
ambv marked this conversation as resolved.
Show resolved Hide resolved
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
ambv marked this conversation as resolved.
Show resolved Hide resolved
"Re-run failed jobs". The button will only be present when all other jobs
finished running.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only applies to core devs and triagers?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's also true for the previous paragraph (about merging in main).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any contributor can merge main into their PR, for example: python/cpython#105984 (commits)


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
====================
Expand Down Expand Up @@ -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
1 change: 1 addition & 0 deletions triage/triaging.rst
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ you can help by making sure the pull request:
* includes a :ref:`NEWS entry <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 <keeping-ci-green>`

Doing all of this allows core developers and :ref:`triagers <triage-team>`
to more quickly look for subtle issues that only people with extensive
Expand Down
Loading