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.
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
