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.

Sign up for GitHub

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

Jump to bottom

Clarify one-time setup for backporting #16016

Merged
merged 2 commits into from
Jun 2, 2021
Merged

Clarify one-time setup for backporting #16016

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2ae49ce
docs: Update requirements for backporting
christarazi May 4, 2021
90e27ea
docs: Recommend use of dev VM for backporting
christarazi May 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 8 additions & 7 deletions Documentation/contributing/release/backports.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,11 @@ process for backporting these PRs:
One-time Setup
~~~~~~~~~~~~~~

#. The scripts referred to below need to be run on Linux, they do not
work on macOS. You can use the cilium dev VM for this, but you need
to configure git to have your name and email address to be used in
the commit messages:
#. The scripts referred to below need to be run on Linux, they do not work on
macOS. It is recommended to use the dev VM (``contrib/vagrant/start.sh``),
as it will have all the correct versions of dependencies / libraries. Once
you have a machine ready, you need to configure git to have your name and
email address to be used in the commit messages:

.. code-block:: bash

Expand Down Expand Up @@ -101,11 +102,11 @@ One-time Setup
+--------------------------------------------------------------+-----------+---------------------------------------------------------+
| jq | Yes | N/A (OS-specific) |
+--------------------------------------------------------------+-----------+---------------------------------------------------------+
| python3 | No | `Python Downloads <https://www.python.org/downloads/>`_ |
| python3 | Yes | `Python Downloads <https://www.python.org/downloads/>`_ |
+--------------------------------------------------------------+-----------+---------------------------------------------------------+
| `PyGithub <https://pypi.org/project/PyGithub/>`_ | No | ``pip3 install PyGithub`` |
| `PyGithub <https://pypi.org/project/PyGithub/>`_ | Yes | ``pip3 install PyGithub`` |
+--------------------------------------------------------------+-----------+---------------------------------------------------------+
| `Github hub CLI (>= 2.8.3) <https://github.com/github/hub>`_ | No | N/A (OS-specific) |
| `Github hub CLI (>= 2.8.3) <https://github.com/github/hub>`_ | Yes | N/A (OS-specific) |
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that hub is required. At least I haven't had to use it since I created the PR by hand. PyGithub is for sure required for marking PRs else it's really tedious

Copy link
Member Author

Choose a reason for hiding this comment

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

If you grep for hub, it's definitely used. It opens the backport PRs.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, hub is used.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think my initial comment was read properly.

Yes, it's used if you run contrib/backporting/submit-backport. However that itself is not required since the docs branch into:

  • use submit-backport (hub required)
  • open PR manually (and copy over the contents of the file)

If we mark hub as required (it's annoying to install on ubuntu after snap install deprecation) we should remove the "create PR manually" section, or not mark hub as required.

Copy link
Member

Choose a reason for hiding this comment

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

As an additional data point, I opened the backport PR just fine today without hub purely following the docs.

Copy link
Member

Choose a reason for hiding this comment

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

I'd also be in favor of removing the "manual" section. There are regularly backport PRs opened against master, sending tons of notifications to everyone in the process, and without the proper label updates.

If we containerize hub, we might as well include all the backporting scripts.

Copy link
Member

Choose a reason for hiding this comment

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

If we containerize hub, we might as well include all the backporting scripts.

That is even better.

Copy link
Member

Choose a reason for hiding this comment

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

Fwiw, I never used hub, always created PRs manually and I don't recall having issues.

@rolinh the most common issue is accidentally creating the backport PR against the wrong branch (ie opening the PR against master instead of the target branch). Beyond that, there's also making sure the labels are set correctly for the backported PRs, and making sure that the PR description contains the summary of the backported PRs and importantly, the section with the command to update the backports to "done" once the backport PR is merged, which is not only for label management but is also used to correlate the backport + original PRs in release notes. The script handles all of these bits and pieces for you so you don't need to make sure you line them up correctly.

Copy link
Contributor

Choose a reason for hiding this comment

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

...opening the PR against master instead of the target branch

It happen to me several times. Fortunately github allows changing branches after PR was open, however reviewer requests end-up being messed up.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've been using a container every time, and recently published the images here:
https://hub.docker.com/repository/docker/errordeveloper/cilium-backport-tools

I'd be happy to turn that one-off image into something we can all use, as it sounds this would be of interest to people.

+--------------------------------------------------------------+-----------+---------------------------------------------------------+

Verify your machine is correctly configured by running
Expand Down