[Merge] ~vorlon/ubuntu-dev-tools:pm-helper into ubuntu-dev-tools:main
Bryce Harrington
mp+444677 at code.launchpad.net
Wed Jun 14 00:49:52 UTC 2023
Review: Needs Fixing
Cool, hopefully my suggestions inline are useful. I look forward to how this develops.
Diff comments:
> diff --git a/pm-helper b/pm-helper
> new file mode 100755
> index 0000000..7063675
> --- /dev/null
> +++ b/pm-helper
> @@ -0,0 +1,145 @@
> +#!/usr/bin/python3
> +# Find the next thing to work on for proposed-migration
> +# Copyright (C) 2023 Canonical Ltd.
> +# Author: Steve Langasek <steve.langasek at ubuntu.com>
> +
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License, version 3.
> +
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> +# General Public License for more details.
> +
> +# You should have received a copy of the GNU General Public License
> +# along with this library; if not, write to the Free Software
> +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> +# 02110-1301 USA
> +
> +import lzma
> +from optparse import OptionParser
You might consider ArgParse since OptionParser is deprecated now for python 3. I found it pretty analogous, even though not a drop-in replacement.
> +import sys
> +import webbrowser
> +import yaml
> +
> +from launchpadlib.launchpad import Launchpad
> +
> +from ubuntutools.utils import get_url
> +
> +
> +# proposed-migration is only concerned with the devel series; unlike other
> +# tools, don't make this configurable
> +excuses_url = 'https://ubuntu-archive-team.ubuntu.com/proposed-migration/' \
> + + 'update_excuses.yaml.xz'
> +
> +
> +def get_proposed_version(excuses, package):
> + for k in excuses['sources']:
> + if k['source'] == package:
> + excuse = k
> + break
> +
> + if not excuse:
> + return False
> +
> + return excuse['new-version']
I'd return None rather than False on the error; both resolve to untrue but None better communicates that there's not a defined value to be returned.
I ran:
$ ./pm-helper apache2
And got an error here:
UnboundLocalError: local variable 'excuse' referenced before assignment
Which suggests that 'excuse = None' should be placed at the top of the function. However, I think you could restructure the code a bit like this:
def get_proposed_version(excuses, package):
for k in excuses['sources']:
if k['source'] == package:
return k.get('new-version')
return None
> +
> +
> +def claim_excuses_bug(launchpad, bug, package):
> + print("LP: #%d: %s" % (bug.id, bug.title))
> + ubuntu = launchpad.distributions['ubuntu']
> + series = ubuntu.current_series.fullseriesname
> +
> + for task in bug.bug_tasks:
> + # targeting to a series doesn't make the default task disappear,
> + # it just makes it useless
> + if task.bug_target_name == "%s (%s)" % (package, series):
> + our_task = task
> + break
> + elif task.bug_target_name == "%s (Ubuntu)" % package:
> + our_task = task
> +
> + if our_task.assignee == launchpad.me:
> + print("Bug already assigned to you.")
> + return True
> + elif our_task.assignee:
> + print("Currently assigned to %s" % our_task.assignee.name)
> +
> + print('''Do you want to claim this bug? [yN] ''', end="")
> + sys.stdout.flush()
> + response = sys.stdin.readline()
> + if response.strip().lower().startswith('y'):
> + our_task.assignee = launchpad.me
> + our_task.lp_save()
> + return True
> +
> + return False
> +
> +
> +def create_excuses_bug(launchpad, package, version):
> + print("Will open a new bug")
> + bug = launchpad.bugs.createBug(
> + title = 'proposed-migration for %s %s' % (package, version),
> + tags = ('update-excuse'),
> + target = 'https://api.launchpad.net/devel/ubuntu/+source/%s' % package,
> + description = '%s %s is stuck in -proposed.' % (package, version)
> + )
You'll want to do some exception handling around this, for the case that the user typos the package name or passes something not present in -devel. I.e., this will throw a stack trace:
$ ./pm-helper foobar
...
---
Response body:
---
b'Package foobar not published in Ubuntu'
---
So I think you'll want something like:
def create_excuses_bug(launchpad, package, version):
print("Will open a new bug")
try:
bug = launchpad.bugs.createBug(
title = 'proposed-migration for %s %s' % (package, version),
tags = ('update-excuse'),
target = 'https://api.launchpad.net/devel/ubuntu/+source/%s' % package,
description = '%s %s is stuck in -proposed.' % (package, version)
)
except BadRequest as e:
sys.stderr.write(f"Error: {e.content.decode('utf-8')}\n")
return None
...
Which will give a nice error:
$ ./pm-helper foobar
Will open a new bug
Error: Package foobar not published in Ubuntu
> +
> + task = bug.bug_tasks[0]
> + task.assignee = launchpad.me
> + task.lp_save()
> +
> + print("Opening %s in browser" % bug.web_link)
> + webbrowser.open(bug.web_link)
> + return bug
> +
> +
> +def find_excuses_bugs(launchpad, package):
I'd suggest naming this has_excuses_bugs (which will imply a boolean return). The find_ prefix often implies returning the found object or list.
> + ubuntu = launchpad.distributions['ubuntu']
> + tasks = ubuntu.getSourcePackage(name=package).searchTasks(
> + tags=['update-excuse'], order_by=['id'])
> +
If the program is run with a non-existent package it'll fail here since getSourcePackage() will return None. For example:
$ ./pm-helper foobarasdfasdf
Finding excuses for foobarasdfasdf
None
Traceback (most recent call last):
File "/home/bryce/src/Ubuntu/UbuntuDevTools/ubuntu-dev-tools/./pm-helper", line 160, in <module>
sys.exit(main())
File "/home/bryce/src/Ubuntu/UbuntuDevTools/ubuntu-dev-tools/./pm-helper", line 142, in main
if not find_excuses_bugs(options.launchpad, args[0]):
File "/home/bryce/src/Ubuntu/UbuntuDevTools/ubuntu-dev-tools/./pm-helper", line 101, in find_excuses_bugs
tasks = pkg.searchTasks(
AttributeError: 'NoneType' object has no attribute 'searchTasks'
So you could do something like this:
def has_excuses_bugs(launchpad, package):
ubuntu = launchpad.distributions['ubuntu']
pkg = ubuntu.getSourcePackage(name=package)
if not pkg:
raise ValueError(f"No such source package: {package}")
tasks = pkg.searchTasks(
tags=['update-excuse'], order_by=['id'])
bugs = [task.bug for task in tasks]
if not bugs:
return False
And then catch the ValueError down where has_excuses_bug() gets called (see below).
(I find it humorous that `./pm-helper foobar` worked...)
> + bugs = [task.bug for task in tasks]
> + if not bugs:
> + return False
> +
> + if len(bugs) == 1:
> + print("There is 1 open update-excuse bug against %s" % package)
> + else:
> + print("There are %d open update-excuses bugs against %s" \
> + % (len(bugs), package))
You probably want to phrase it as "open update-excuse bugs".
Also, you may find it convenient to use python's new f-string support:
print(f"There are {len(bugs)} open update-excuses bugs against {package}")
> +
> + for bug in bugs:
> + if claim_excuses_bug(launchpad, bug, package):
> + return True
> +
> + return True
> +
> +
> +def main():
> + parser = OptionParser(usage="usage: %prog [options] [package]")
> + parser.add_option(
> + "-l", "--launchpad", dest="launchpad_instance", default="production")
> + parser.add_option(
> + "-v", "--verbose", default=False, action="store_true",
> + help="be more verbose (redundant in --dry-run mode)")
> + options, args = parser.parse_args()
> +
> + options.launchpad = Launchpad.login_with(
> + "pm-helper", options.launchpad_instance, version="devel")
> +
> + f = get_url(excuses_url, False)
> + lzma_f = lzma.open(f)
> + excuses = yaml.load(lzma_f, Loader=yaml.CSafeLoader)
> + lzma_f.close()
You can use a context manager for the open, i.e.:
f = get_url(excuses_url, False)
with lsma.open(f) as lzma_f:
excuses = yaml.load(lzma_f, Loader=yaml.CSafeLoader)
> +
> + if args:
> + if not find_excuses_bugs(options.launchpad, args[0]):
> + create_excuses_bug(options.launchpad, args[0],
> + get_proposed_version(excuses, args[0]))
A user prompt here might be friendly to add at some point, for example maybe something like:
if args:
try:
if not has_excuses_bugs(options.launchpad, args[0]):
if not options.force:
print(f"Open a new update-excuse bug for {args[0]}?")
answer = None
try:
answer = input("Proceed? [y/N]")
except:
sys.stderr.write("Aborted\n")
return 1
if answer.lower() not in ("y", "yes"):
return 1
create_excuses_bug(options.launchpad, args[0],
get_proposed_version(excuses, args[0]))
except ValueError as e:
sys.stderr.write(f"{e}\n")
else:
pass # for now
This could be refactored to look nicer, of course. For instance you might combine the prompting logic with the claim prompt into a helper routine.
> + else:
> + pass # for now
> +
> +
> +if __name__ == '__main__':
> + sys.exit(main())
--
https://code.launchpad.net/~vorlon/ubuntu-dev-tools/+git/ubuntu-dev-tools/+merge/444677
Your team Ubuntu Development Team is requested to review the proposed merge of ~vorlon/ubuntu-dev-tools:pm-helper into ubuntu-dev-tools:main.
More information about the Ubuntu-reviews
mailing list