Clarify one-time setup for backporting #16016
Conversation
christarazi
commented
May 4, 2021
christarazi
commented
- docs: Fix GitHub requirements for backporting
- docs: Recommend use of dev VM for backporting
christarazi
added
area/documentation
maintainer-s-little-helper
bot
added
the
dont-merge/needs-release-note-label
maintainer-s-little-helper
bot
removed
the
dont-merge/needs-release-note-label
christarazi
changed the title
| 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 recommend to use the dev VM (``contrib/vagrant/start.sh``), as |
There was a problem hiding this comment.
Why is that? We call out the required dependency packages along with their versions, no?
nit: *recommended
There was a problem hiding this comment.
A user can spin up their own vagrant VM which has old Python libraries that cause problems with PyGithub for example. We might as well point them at a well-maintained VM that is essentially guaranteed to work.
There was a problem hiding this comment.
We call out the required dependency packages along with their versions
You marked them as required in your 1st commit?
We might as well point them at a well-maintained VM
We already do, no?
There was a problem hiding this comment.
Yes, we call out the dependencies, but that doesn't guarantee that the underlying Python libraries on the system for example are compatible with the versions of the dependencies we require.
There was a problem hiding this comment.
AFAIR, we still need to install couple of packages for the backporting process. Did you try running all the backporting scripts in a dev VM?
There was a problem hiding this comment.
I've not used the dev VM for backporting because I'm already on Linux. I just want to make it easier for those who do. I believe you use the dev VM for backporting--do you remember what additional packages those are? Can you add them to the list that I've modified? I figured because we already have the dev VM used by many others, it's going to be the most maintained, and since it'll likely be used for development purposes anyway, why not recommend it. This is easier from a maintenance and support point of view, so that we avoid issues where people are bringing their own environment, when one already exists.
christarazi
force-pushed
the
pr/christarazi/fix-requirements-pygithub
branch
from
f7b7296
to
d82ad6b
Compare
| +--------------------------------------------------------------+-----------+---------------------------------------------------------+ | ||
| | `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) | |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
If you grep for hub, it's definitely used. It opens the backport PRs.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
As an additional data point, I opened the backport PR just fine today without hub purely following the docs.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
If we containerize hub, we might as well include all the backporting scripts.
That is even better.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
...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.
There was a problem hiding this comment.
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.
| @@ -103,9 +104,9 @@ One-time Setup | |||
| +--------------------------------------------------------------+-----------+---------------------------------------------------------+ | |||
| | python3 | No | `Python Downloads <https://www.python.org/downloads/>`_ | | |||
There was a problem hiding this comment.
| | python3 | No | `Python Downloads <https://www.python.org/downloads/>`_ | | |
| | python3 | Yes | `Python Downloads <https://www.python.org/downloads/>`_ | |
Based on PyGithub being required below...
|
It looks like a container image would be helpful to quite a few folks, I'll try to open a PR later on to get the ball rolling. |
christarazi
added
the
ready-to-merge
maintainer-s-little-helper
bot
moved this from Needs backport from master
to Backport done to v1.8
in 1.8.11
