launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing

Thanks for working on this, and apologies for the delay in reviewing it.

This looks OK as far as it goes, but I'd expect some test changes as well.  lib/lp/registry/stories/webservice/xx-distribution.txt and lib/lp/registry/stories/webservice/xx-distribution-mirror.txt will certainly need at least basic changes due to the newly-exported fields, and it would be good to add suitable tests to lib/lp/registry/browser/tests/distributionmirror-views.txt and lib/lp/registry/doc/distribution-mirror.txt.

Let us know if you have any difficulties running the test suite, and we can walk you through it.

Diff comments:

> 
> === modified file 'lib/lp/registry/interfaces/distributionmirror.py'
> --- lib/lp/registry/interfaces/distributionmirror.py	2015-10-26 14:54:43 +0000
> +++ lib/lp/registry/interfaces/distributionmirror.py	2019-04-13 16:32:28 +0000
> @@ -342,6 +348,11 @@
>          allowed_schemes=['http'], allow_userinfo=False,
>          allow_query=False, allow_fragment=False, trailing_slash=True,
>          description=_('e.g.: http://archive.ubuntu.com/ubuntu/')))
> +    https_base_url = exported(DistroMirrorHTTPSURIField(
> +        title=_('HTTPS URL'), required=False, readonly=False,
> +        allowed_schemes=['https'], allow_userinfo=False,
> +        allow_query=False, allow_fragment=False, trailing_slash=True,
> +        description=_('e.g.: https://archive.ubuntu.com/ubuntu/')))

It might be best to drop the description field here for now, and perhaps leave an XXX comment about it; it's a bit confusing to include it given that archive.ubuntu.com doesn't yet support HTTPS.

>      ftp_base_url = exported(DistroMirrorFTPURIField(
>          title=_('FTP URL'), required=False, readonly=False,
>          allowed_schemes=['ftp'], allow_userinfo=False,


-- 
https://code.launchpad.net/~abrody/launchpad/https-mirror/+merge/362903
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.