launchpad-reviewers team mailing list archive

Thread
Date
Re: [Merge] lp:~maxiberta/launchpad/snap-pocket-help into lp:launchpad

To: mp+300626@xxxxxxxxxxxxxxxxxx
From: Colin Watson <cjwatson@xxxxxxxxxxxxx>
Date: Thu, 21 Jul 2016 14:26:53 -0000
In-reply-to: <20160720152824.30218.68498.launchpad@ackee.canonical.com>
Reply-to: mp+300626@xxxxxxxxxxxxxxxxxx
Sender: bounces@xxxxxxxxxxxxx
Review: Approve



Diff comments:

> === modified file 'lib/lp/snappy/browser/snap.py'
> --- lib/lp/snappy/browser/snap.py	2016-07-16 07:46:23 +0000
> +++ lib/lp/snappy/browser/snap.py	2016-07-20 15:26:47 +0000
> @@ -242,11 +242,17 @@
>              Choice(vocabulary='SnapDistroArchSeries'),
>              title=u'Architectures', required=True)
>          pocket = Choice(
> -            title=u'Pocket', vocabulary=PackagePublishingPocket, required=True)
> +            title=u'Pocket', vocabulary=PackagePublishingPocket, required=True,
> +            description=u'The set of packages with which this snap package '
> +                'should be built.')

It's rather confusing to describe this as a set of packages, because it sort of implies that you might be able to pass in an actual set of package names or something which of course you can't.

I don't see any clearer description of a pocket anywhere in Launchpad, but how about something like this?

  The update stream within the source distribution series to use when building the snap package.

>  
>      custom_widget('archive', SnapArchiveWidget)
>      custom_widget('distro_arch_series', LabeledMultiCheckBoxWidget)
>  
> +    help_links = {
> +        "pocket": u"/+help-snappy/snap-build-pocket.html",
> +    }

LP's Python style for multi-line displays is normally to indent the closing brace to the same level as the elements within the display (https://dev.launchpad.net/PythonStyleGuide#Multiline_braces).  I find it a little bit weird in some ways, but it would be best to be consistent.

> +
>      @property
>      def cancel_url(self):
>          return canonical_url(self.context)
> 
> === added file 'lib/lp/snappy/help/snap-build-pocket.html'
> --- lib/lp/snappy/help/snap-build-pocket.html	1970-01-01 00:00:00 +0000
> +++ lib/lp/snappy/help/snap-build-pocket.html	2016-07-20 15:26:47 +0000
> @@ -0,0 +1,33 @@
> +<html>
> +  <head>
> +    <title>Pocket for snap builds</title>
> +    <link rel="stylesheet" type="text/css"
> +          href="/+icing/yui/cssreset/reset.css" />
> +    <link rel="stylesheet" type="text/css"
> +          href="/+icing/yui/cssfonts/fonts.css" />
> +    <link rel="stylesheet" type="text/css"
> +          href="/+icing/yui/cssbase/base.css" />
> +    <style type="text/css">
> +      p { margin-bottom: 0.5em }
> +    </style>
> +  </head>
> +  <body>
> +    <h1>Pocket for snap builds</h1>
> +
> +    <p>Package repositories may contain many versions of a package, each in a different subdirectory or <em>pocket</em>.</p>

"Subdirectory" is a bit confusing here, for multiple reasons:

 * Different series are in different directories exactly as much as different pockets within a series are.
 * While the indexes are in different directories by series/pocket, the actual package files are not.

How about something like:

  Each distribution series in a package repository is divided into several parts, called <em>pockets</em>.  These refer to different update streams that users of that series may wish to use.  You can similarly choose to build snap packages based on any of those different update streams.

> +    <p>If unsure, choose <strong>Updates</strong>.</p>
> +    <dl>
> +        <dt><strong>Release</strong></dt>
> +        <dd>The current stable Ubuntu release. Package versions are frozen and do not change.</dd>

The release pocket is orthogonal to whether it's the current stable Ubuntu release.  At the time of writing, xenial is the current stable Ubuntu release, but wily, xenial, yakkety etc. all have their own release pockets.

Although not all of PackagePublishingPocket's descriptions are suitable for this help page, in this case I'd suggest borrowing text from the description of PackagePublishingPocket.RELEASE:

  The package versions that were published when the distribution release was made.

> +        <dt><strong>Security</strong></dt>
> +        <dd>Patches for security vulnerabilities in Ubuntu packages. They are managed by the Ubuntu Security Team and are designed to change the behavior of the package as little as possible -- in fact, the minimum required to resolve the security problem. As a result, they tend to be very low-risk to apply and all users are urged to apply security updates. Packages in <em>Security</em> are built with dependencies from <em>Release</em> and <em>Security</em>.</dd>

I think this has a bit too much in the way of specifics about Ubuntu processes; we try to avoid hardcoding those into Launchpad wherever possible.  The first sentence of PackagePublishingPocket.SECURITY is probably good here: "Package versions containing security fixes for the released distribution."  It's fine to keep the last sentence of your version, though.

> +        <dt><strong>Updates</strong></dt>
> +        <dd>Updates for serious bugs in Ubuntu packaging that do not affect the security of the system. Packages in <em>Updates</em> are copied from <em>Proposed</em> once they have been tested.</dd>

Remove "in Ubuntu packaging" (too much Ubuntu-specific detail and not always accurate).

Also, some packages in updates are in fact copied from security instead.

> +        <dt><strong>Proposed</strong></dt>
> +        <dd>The testing area for updates. This repository is recommended only to those interested in helping to test updates and provide feedback. Packages in <em>Proposed</em> are built with dependencies from <em>Release</em>, <em>Security</em>, <em>Updates</em> and <em>Proposed</em>.</dd>
> +        <dt><strong>Backports</strong></dt>
> +        <dd>As the name states, these are unsupported new versions of packages which have been backported to an older release. Packages may contain new features, may introduce new interfaces, and bugs. Packages in <em>Backports</em> are built with dependencies from <em>Release</em>, <em>Security</em> and <em>Backports</em>.</dd>

Insert "Updates" after "Security" (see lp.soyuz.adapters.archivedependencies:pocket_dependencies).

> +    </dl>
> +  </body>
> +</html>
> +
> 
> === modified file 'lib/lp/snappy/interfaces/snap.py'
> --- lib/lp/snappy/interfaces/snap.py	2016-07-15 17:11:09 +0000
> +++ lib/lp/snappy/interfaces/snap.py	2016-07-20 15:26:47 +0000
> @@ -381,8 +381,8 @@
>          title=_("Pocket for automatic builds"),
>          vocabulary=PackagePublishingPocket, required=False, readonly=False,
>          description=_(
> -            "The pocket for which automatic builds of this snap package "
> -            "should be built.")))
> +            "The set of packages with which automatic builds of this snap "
> +            "package should be built.")))

I think this is probably best simply reverted as the API documentation can afford to contain a bit more jargon for the sake of precision, but if you have to change it then consider something like my first comment above.

>  
>      is_stale = Bool(
>          title=_("Snap package is stale and is due to be rebuilt."),


-- 
https://code.launchpad.net/~maxiberta/launchpad/snap-pocket-help/+merge/300626
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.
References

[Merge] lp:~maxiberta/launchpad/snap-pocket-help into lp:launchpad
From: Maximiliano Bertacchini, 2016-07-20