[Merge] lp:~smoser/ubuntu-archive-tools/package-sets into lp:ubuntu-archive-tools

[Robie Basak]

Review: Needs Fixing



Diff comments:

> === modified file 'sru-release'
> --- sru-release	2017-08-30 16:31:00 +0000
> +++ sru-release	2018-06-21 14:41:10 +0000
> @@ -38,6 +39,54 @@
>  
>  from launchpadlib.launchpad import Launchpad
>  
> +PACKAGE_SETS = (
> +    ('linux-hwe', 'linux-meta-hwe'),
> +    ('linux', 'linux-meta'),
> +    ('grub2', 'grub2-signed'),
> +)

Could you explain this data structure in a comment please? In particular, I understand what it means, but nevertheless I don't follow the ordering you're defining here. What does it mean for one thing to be placed after the other? I would expect this to be defining the order in which related stuff has to land ("if you're releasing linux-hwe, you also need to release linux-meta-hwe and it has to go after") but I'm not sure that's what you've implemented here?

PACKAGE_SETS is an unfortunate choice of name. Can we name it something else to differentiate from "packagesets" which already have a meaning in Launchpad (lists of sourse packages for ACL use).

On the use of tuples here, I'm on the side of https://stackoverflow.com/a/626871/478206. But it's a stylistic thing. I don't know what style is enforced in this repo, so I'll leave that to you.

> +
> +
> +def check_package_sets(packages):
> +    """Return a re-ordered list of packages respecting the PACKAGE_SETS
> +    defined above.  If any packages are missing, raise error."""
> +    pkg2set = {}
> +    seen = set()
> +    for pset in PACKAGE_SETS:
> +        for pkg in pset:
> +            if pkg in pkg2set:
> +                raise RuntimeError(
> +                    "Overlapping package sets. %s in %s and %s." %
> +                    (pkg, pset, pkg2set[pkg]))
> +            pkg2set[pkg] = pset
> +    new_pkgs = []
> +    for pkg in packages:
> +        if pkg not in pkg2set:
> +            add = [pkg]
> +        else:
> +            add = list(pkg2set[pkg])
> +        new_pkgs.extend([a for a in add if a not in seen])
> +        seen.update(add)
> +
> +    orig = set(packages)
> +    new = set(new_pkgs)
> +    if orig != new:
> +        raise ValueError(
> +            ("Package list needs packages added:\n   %s\n"
> +             "use --skip-package-set-check to skip this check.\n") %
> +            ' '.join(new.difference(orig)))

This is in practice a very rare case (which is why it's good to put this check into the tool). New SRU team members may be unaware of it. So can we make the message more explicit please? For example:

"The set of packages requested for release are listed as dangerous to release without the simultaneous addition of the following packages to be released at the same time: .... See https://lists.ubuntu.com/archives/ubuntu-devel/2018-June/040380.html for details. Use --skip-package-set-check to override and face hordes of angry users".

Reword as you wish, but my point is to be really really explicit about why it's refusing to run, so that users aren't tempted to override without having understood properly.

> +    return new_pkgs
> +
> +
> +class CheckPackageSets(unittest.TestCase):
> +    def test_expected_linux_order_fixed(self):
> +        self.assertEqual(
> +            ['pkg1', 'linux', 'linux-meta', 'pkg2'],
> +            check_package_sets(['pkg1', 'linux-meta', 'linux', 'pkg2']))
> +
> +    def test_raises_value_error_on_missing(self):
> +        self.assertRaises(
> +            ValueError, check_package_sets, ['pkg1', 'linux'])
> +

I'm mostly relying on the tests here rather than worrying about your algorithm too much. But in that case I think we need tests for: single item (the very common case), and a list that covers more than one set (eg. [linux-hwe, linux-meta-hwe, grub2]).

>  
>  def match_srubugs(options, changesfileurl):
>      '''match between bugs with verification- tag and bugs in changesfile'''


-- 
https://code.launchpad.net/~smoser/ubuntu-archive-tools/package-sets/+merge/348334
Your team Ubuntu Package Archive Administrators is requested to review the proposed merge of lp:~smoser/ubuntu-archive-tools/package-sets into lp:ubuntu-archive-tools.