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

[Scott Moser]

I've addressed your comments.

I don't think this needs to be perfect right now, or cover all potential
problem cases on the first pass.  It can be improved over time.  As it is
right now, it helps to prevent a known problem with no known side effects.


On Wed, Aug 8, 2018 at 5:11 PM Robie Basak <robie.basak at canonical.com>
wrote:

> 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
> You are the owner of lp:~smoser/ubuntu-archive-tools/package-sets.
>

-- 
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.