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

dist/tools: Add script for backporting PR's #8968

Merged
merged 2 commits into from
Feb 12, 2019

Conversation

bergzand
Copy link
Member

@bergzand bergzand commented Apr 17, 2018

Contribution description

This python script provides functionality to easily backport a merged PR to a release.

It relies on having a github API token stored locally to authenticate against github, by default it looks into ~/.githubtoken ~/.riotgithubtoken, but this is configurable.

The script works by fetching information from the supplied (merged) PR. It then looks for the latest release branch. A temporary worktree with a new branch checked out is created based on this release branch. All commit from the PR are then cherry picked into the worktree and the new branch is pushed to origin. It then creates a new pull request with a reference to the old PR on github and optionally puts a comment under the old PR with a reference to the new backport PR.

I haven't been able to test the last few lines of the script (creating the PR and placing the comment) as I did not want to create new backport PR's for 2018.01 :)

Issues/PRs references

#8523

TODO:

  • Make release configurable
  • Copy labels to new PR
  • Check if the supplied PR is already merged
  • Noop mode

@bergzand bergzand added State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: tools Area: Supplementary tools labels Apr 17, 2018
@bergzand bergzand requested review from cladmi and aabadie April 17, 2018 12:20
@miri64
Copy link
Member

miri64 commented Apr 17, 2018

Thank you, I sadly never came to doing that :(

@cladmi
Copy link
Contributor

cladmi commented Apr 17, 2018

Not tested yet. It would be really cool to use it and test for this release.

As discussed irl, I would prefer some subfunctions to split the tasks, as you say there is "fetch", "create worktree", "cherry-pick", "publish PR". Also maybe separate strings operation/sorting and things on github object.

Right now, as most part of the code is below the if __name__ == 'main', it is not checked by flake8, so it would be better to do a main function. I discovered it because of I saw these unused variables (it's the only error).

flake8 backport_pr.py
backport_pr.py:101:5: F841 local variable 'curdir' is assigned to but never used
backport_pr.py:101:14: F841 local variable 'dir_path' is assigned to but never used

We can spend some time together on it and write a user doc.

@bergzand
Copy link
Member Author

Added noop mode to the todo list after some offline discussion

@kaspar030
Copy link
Contributor

I tried it (and felt reckless). Some issues:

  • the github token file reading needs to strip any newlines:
[kaspar@ng backport_pr (pr8968)]$ python3 backport_pr.py -r 2018.04 -c -d /home/kaspar/src/backport 8998
Traceback (most recent call last):
  File "backport_pr.py", line 69, in <module>
    username = g.user.get()[1]['login']
  File "/home/kaspar/.local/lib/python3.6/site-packages/agithub/base.py", line 157, in get
    return self.request('GET', url, None, headers)
  File "/home/kaspar/.local/lib/python3.6/site-packages/agithub/base.py", line 207, in request
    conn.request(method, url, requestBody.process(), headers)
  File "/usr/lib/python3.6/http/client.py", line 1239, in request
    self._send_request(method, url, body, headers, encode_chunked)
  File "/usr/lib/python3.6/http/client.py", line 1280, in _send_request
    self.putheader(hdr, value)
  File "/usr/lib/python3.6/http/client.py", line 1217, in putheader
    raise ValueError('Invalid header value %r' % (values[i],))
ValueError: Invalid header value b'Token foobar\n'

./githubtoken contained the token + newline.

Next problem:

$ python3 backport_pr.py -r 2018.04 -c -d /home/kaspar/src/backport 8998
Fetching for commit: #8998: net/gcoap: increase stack size by sizeof(coap_pkt_t)
found e1f6a5afd3fd27d96a3f86921dd1d860dc7f9727 : net/gcoap: increase stack size by sizeof(coap_pkt_t)
Traceback (most recent call last):
  File "backport_pr.py", line 94, in <module>
    release_name, release_latest = get_latest_release(branches[1])
  File "backport_pr.py", line 33, in get_latest_release
    branch_name = branch['name']
TypeError: string indices must be integers

I stopped here for now.

This is on Arch Linux with python 3.6.

@bergzand
Copy link
Member Author

Thanks for testing, will fix

@bergzand
Copy link
Member Author

Next problem:

Ah, manually selecting a branch is not really well supported yet, by default it selects the latest release by sorting them numerically 😨

@bergzand
Copy link
Member Author

Note: The access token requires public repo access, otherwise the script fails on creating the pull request with a non-descriptive 404.
screenshot_20180423_161147

@miri64
Copy link
Member

miri64 commented Apr 23, 2018

Note: The access token requires public repo access, otherwise the script fails on creating the pull request with a non-descriptive 404.

Can't you catch the 404 and print a help message regarding that?

@bergzand
Copy link
Member Author

Can't you catch the 404 and print a help message regarding that?

Yes of course, I'm a bit dissapointed in that a 404 is returned and not a 403, but they probably have a good reason for that.

@miri64
Copy link
Member

miri64 commented Apr 23, 2018

Yes of course, I'm a bit dissapointed in that a 404 is returned and not a 403, but they probably have a good reason for that.

Most likely object obfuscation. It's in general safer (against brute force attacks) to pretend a resource does not exist than to reveal, that it does but that the requester has no permission to read it.

@cladmi
Copy link
Contributor

cladmi commented Apr 27, 2018

Summary of the irl discussion on changes I would be interested in.

Add informations on getting the token

Github->Settings->Developper settings->Personal access tokens->Generate New
Token

Only following scope is required

repo[public repo]: Access public repositories

Name it, generate and save it to ~/.githubtoken (warning you cannot see it later anymore).

--gitdir

  • Detect it automatically! And document/detect cases where is should be specified (as you said when using worktree)
  • -d gitdir -> say it's RIOTBASE and not the '.git' maybe name is misleading

--comment

  • -c comment -> maybe do the comment by default and have "--no-comment" ?

Separating steps

Instead of a "--noop", I am interested in having separate steps so I can review them:

  • create branch
    • I can verify the commit and all, maybe re-run tests also
    • I am signing these automatically created commits so would like to be able
      to review them :)
  • see the would be created pr title and message and labels
  • upload branch and create pr
  • Add a "--cleanup" option that you can run if something went wrong in the
    middle
  • And of course a lazy solution "do everything"

@cladmi
Copy link
Contributor

cladmi commented Apr 27, 2018

I forgot that to make it work, I disabled this line in my config

[push]
	default = upstream

It was pushing to riot/2018-04-branch instead of the correct branch.
I will re-test it when trying another backport.

@cladmi
Copy link
Contributor

cladmi commented May 2, 2018

For the push default the following patch made it work:

git diff
diff --git a/dist/tools/backport_pr/backport_pr.py b/dist/tools/backport_pr/backport_pr.py
index 676124b..f8463bc 100644
--- a/dist/tools/backport_pr/backport_pr.py
+++ b/dist/tools/backport_pr/backport_pr.py
@@ -164,7 +164,7 @@ def main():
     # Push to github
     print("Pushing branch {} to origin".format(new_branch))
     if not args.noop:
-        repo.git.push('origin', new_branch)
+        repo.git.push('origin', '{0}:{0}'.format(new_branch))
     # Delete worktree
     print("Pruning temporary workdir at {}".format(worktree_dir))
     _delete_worktree(repo, worktree_dir)

@bergzand
Copy link
Member Author

bergzand commented May 2, 2018

@cladmi Thanks for investigating!

@miri64
Copy link
Member

miri64 commented May 4, 2018

Please make the script executable.

@miri64
Copy link
Member

miri64 commented May 4, 2018

Some more remarks:

  • some in-line doc how the keyfile is supposed to look like would be nice (maybe even generate it with help of input()).
  • I'm pretty sure I activated the "Public Repo" access for the token, nevertheless I got the exception (maybe this was already discussed?)

@miri64
Copy link
Member

miri64 commented May 4, 2018

(minor nit: I think the name ~/.githubtoken as default token file is a little bit too generic, maybe ~/.githubbprtoken or something so it is associated with the backport script)

@bergzand
Copy link
Member Author

bergzand commented May 7, 2018

(minor nit: I think the name ~/.githubtoken as default token file is a little bit too generic, maybe ~/.githubbprtoken or something so it is associated with the backport script)

If a less generic name is preferred, I'd rather name it something with riot in the name: ~/.riotgithubtoken or something

Please make the script executable.

Should be fixed.

I'm pretty sure I activated the "Public Repo" access for the token, nevertheless I got the exception (maybe this was already discussed?)

Not sure why. I'd have to investigate this, the notification means that for some reason the pull request could not be created, but this could be anything.

@miri64
Copy link
Member

miri64 commented Jul 17, 2018

If a less generic name is preferred, I'd rather name it something with riot in the name: ~/.riotgithubtoken or something

👍

@cladmi
Copy link
Contributor

cladmi commented Jul 18, 2018

Fixed-up the new label for "backporting-needed" #9591 (comment)

@cladmi
Copy link
Contributor

cladmi commented Jul 19, 2018

Renamed the token file.

@miri64
Copy link
Member

miri64 commented Jul 19, 2018

Some doc which permissions are required for the token would be nice ;-).

@miri64
Copy link
Member

miri64 commented Jul 19, 2018

The YYYY.MM/branch-name naming scheme poses a problem for post-release backports, as the naming will collide with the release tag (git creates a file YYYY.MM in .git/refs/heads/, when the backport branch is than created it will try create a directory YYYY.MM in .git/refs/heads/ which name will collide with the file representing the release tag).

@jia200x jia200x added this to the Release 2018.10 milestone Oct 10, 2018
@cladmi
Copy link
Contributor

cladmi commented Jan 25, 2019

If we start looking into the script quality, I would also point the pylint output:

pylint ./dist/tools/backport_pr/backport_pr.py
************* Module backport_pr
dist/tools/backport_pr/backport_pr.py:111:6: W0511: TODO: exception handling (fixme)
dist/tools/backport_pr/backport_pr.py:187:0: C0330: Wrong hanging indentation (remove 4 spaces).
                'title': "{} [backport {}]".format(pulldata['title'],
            |   ^ (bad-continuation)
dist/tools/backport_pr/backport_pr.py:189:0: C0330: Wrong hanging indentation (remove 4 spaces).
                'head': '{}:{}'.format(username, new_branch),
            |   ^ (bad-continuation)
dist/tools/backport_pr/backport_pr.py:190:0: C0330: Wrong hanging indentation (remove 4 spaces).
                'base': release_fullname,
            |   ^ (bad-continuation)
dist/tools/backport_pr/backport_pr.py:191:0: C0330: Wrong hanging indentation (remove 4 spaces).
                'body': "# Backport of #{}\n\n{}".format(args.PR,
            |   ^ (bad-continuation)
dist/tools/backport_pr/backport_pr.py:193:0: C0330: Wrong hanging indentation (remove 4 spaces).
                'maintainer_can_modify': True
            |   ^ (bad-continuation)
dist/tools/backport_pr/backport_pr.py:1:0: C0111: Missing module docstring (missing-docstring)
dist/tools/backport_pr/backport_pr.py:12:0: E0401: Unable to import 'agithub.GitHub' (import-error)
dist/tools/backport_pr/backport_pr.py:34:0: C0103: Argument name "pr" doesn't conform to snake_case naming style (invalid-name)
dist/tools/backport_pr/backport_pr.py:45:11: C1801: Do not use `len(SEQUENCE)` to determine if a sequence is empty (len-as-condition)
dist/tools/backport_pr/backport_pr.py:47:11: C1801: Do not use `len(SEQUENCE)` to determine if a sequence is empty (len-as-condition)
dist/tools/backport_pr/backport_pr.py:70:0: R1710: Either all return statements in a function should return an expression, or none of them should. (inconsistent-return-statements)
dist/tools/backport_pr/backport_pr.py:82:0: C0111: Missing function docstring (missing-docstring)
dist/tools/backport_pr/backport_pr.py:82:0: R0914: Too many local variables (28/15) (too-many-locals)
dist/tools/backport_pr/backport_pr.py:110:4: C0103: Variable name "g" doesn't conform to snake_case naming style (invalid-name)
dist/tools/backport_pr/backport_pr.py:186:8: C0103: Variable name "pr" doesn't conform to snake_case naming style (invalid-name)
dist/tools/backport_pr/backport_pr.py:82:0: R0912: Too many branches (16/12) (too-many-branches)
dist/tools/backport_pr/backport_pr.py:82:0: R0915: Too many statements (80/50) (too-many-statements)
dist/tools/backport_pr/backport_pr.py:14:0: C0411: standard import "import argparse" should be placed before "from agithub.GitHub import GitHub" (wrong-import-order)
dist/tools/backport_pr/backport_pr.py:15:0: C0411: standard import "import os" should be placed before "from agithub.GitHub import GitHub" (wrong-import-order)
dist/tools/backport_pr/backport_pr.py:16:0: C0411: standard import "import os.path" should be placed before "from agithub.GitHub import GitHub" (wrong-import-order)
dist/tools/backport_pr/backport_pr.py:17:0: C0411: standard import "import shutil" should be placed before "from agithub.GitHub import GitHub" (wrong-import-order)
dist/tools/backport_pr/backport_pr.py:18:0: C0411: standard import "import sys" should be placed before "from agithub.GitHub import GitHub" (wrong-import-order)

------------------------------------------------------------------
Your code has been rated at 8.00/10 (previous run: 8.00/10, +0.00)

And enable the --max-complexity to flake8. I have seen other people use a value of 10 as default.

flake8 --max-complexity=8 ./dist/tools/backport_pr/backport_pr.py
./dist/tools/backport_pr/backport_pr.py:82:1: C901 'main' is too complex (16)

So we could still refine the script I guess.

@cladmi
Copy link
Contributor

cladmi commented Jan 25, 2019

I noticed I rebased on a old master, so just rebased on a new one.

@miri64
Copy link
Member

miri64 commented Jan 25, 2019

Why rebase at all if there is no conflict ;-)?

@cladmi
Copy link
Contributor

cladmi commented Jan 25, 2019

I wanted to be based on the branch where compile_and_test_for_board was included to take the tox.ini file and have it as reference.

@miri64
Copy link
Member

miri64 commented Jan 25, 2019

I wanted to be based on the branch where compile_and_test_for_board was included to take the tox.ini file and have it as reference.

Tox?

@cladmi
Copy link
Contributor

cladmi commented Jan 25, 2019

https://tox.readthedocs.io

@miri64 miri64 dismissed their stale review January 25, 2019 19:42

Spelling was fixed.

@cladmi
Copy link
Contributor

cladmi commented Jan 28, 2019

Before somebody gets crazy when refactoring into functions the Github object behaves quick strangely and should alway be used with the full g.repos[ORG][REPO].pulls[args.PR] syntax. If you re-use a g.repos[ORG][REPO] object, things gets complicated as doing pulls[] does modify the original object. (oh don't ask why, please don't ask why)

@bergzand
Copy link
Member Author

Before somebody gets crazy when refactoring into functions the Github object behaves quick strangely and should alway be used with the full g.repos[ORG][REPO].pulls[args.PR] syntax. If you re-use a g.repos[ORG][REPO] object, things gets complicated as doing pulls[] does modify the original object. (oh don't ask why, please don't ask why)

I've also noticed this once. The whole Github object (or rather the agithub module) is a weird automatic API object that generates API requests based on how you access the object.

@cladmi
Copy link
Contributor

cladmi commented Jan 28, 2019

I've also noticed this once. The whole Github object (or rather the agithub module) is a weird automatic API object that generates API requests based on how you access the object.

Why would not a getter be allowed to modify an object? I think it is a really nice design pattern ><

@miri64
Copy link
Member

miri64 commented Feb 5, 2019

Ok, let's not over-engineer this helper script. Further refactoring and improvements can also be done in follow-up PRs. @bergzand @cladmi, please squash.

Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

(This means re-ACK. @cladmi addressed my change request and added tox support since then, so nothing was drastically changed)

@miri64
Copy link
Member

miri64 commented Feb 5, 2019

(+ I just re-tested this script by opening #10954 ;-))

if not pulldata['merged']:
print("Original PR not yet merged")
exit(0)
print("Fetching for commit: #{}: {}".format(args.PR, pulldata['title']))
Copy link
Member

Choose a reason for hiding this comment

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

One thing I noticed here: if I have a multi-line commit message it shows the whole message, not just the summary line.

@miri64
Copy link
Member

miri64 commented Feb 12, 2019

Ping for squashing btw.

bergzand and others added 2 commits February 12, 2019 15:34
This script provides functionality to easily backport a merged pull request to
a release branch.

It relies of having a `github` API token stored in `~/.riotgithubtoken` by
default.

The script works by fetching information from the supplied **merged** pull
request. It then looks for the last release branch.
A temporary git `worktree` with a new branch is created based on this release
branch. All commits from the pull request are then cherry-picked into this
branch which is then pushed to `origin`.
It then creates a new pull request on `github` with a reference to the original
pull request. It optionally puts a comment under the original pull request to
the new backport pull request.

Co-authored-by: Gaëtan Harter <[email protected]>
Adapt tox file from `compile_and_test_for_board.py`.

`tox` is still not working without errors but gives way to start
refactoring.
@cladmi
Copy link
Contributor

cladmi commented Feb 12, 2019

I squashed and rebased. I in-lined a fix for Wrong hanging indentation warnings.

The tox script is still showing errors but gives things that still need to be improved.

@miri64
Copy link
Member

miri64 commented Feb 12, 2019

Ok, let's fix those in follow-ups.

@miri64 miri64 merged commit a37ba3c into RIOT-OS:master Feb 12, 2019
@bergzand
Copy link
Member Author

@cladmi Thank you for managing this PR!

@cladmi
Copy link
Contributor

cladmi commented Feb 12, 2019

@bergzand you wrote all the logic, I only handled the fiddling which was easier :)

@cladmi
Copy link
Contributor

cladmi commented Feb 12, 2019

So thanks to you !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants