Code review comment for lp:~jpds/launchpad/fix_361650_model_changes

Hi Jonathan.

I have some trivial suggestions to improve this branch. I think the implementation
is good and ready to land.

I can land the branch at the end of this week after these changes are made.

make lint reported these problems that need fixing:

lib/lp/registry/interfaces/distributionmirror.py
    339: [C0301] Line too long (83/78)
    436: [W0311] Bad indentation. Found 7 spaces, expected 8

> === modified file 'lib/lp/registry/doc/distribution-mirror.txt'
> --- lib/lp/registry/doc/distribution-mirror.txt 2009-12-09 10:03:20 +0000
> +++ lib/lp/registry/doc/distribution-mirror.txt 2010-03-29 15:54:12 +0000

@@ -814,3 +814,107 @@

> +Country DNS mirrors
> +-------------------
> +
> +Country DNS mirrors are mirrors which have been assigned $CC.archive.ubuntu.com
> +or $CC.releases.ubuntu.com. These assignments are tracked in Launchpad.

Wrap the narrative at 78 characters.

> + >>> login('<email address hidden>')
> + >>> ubuntu_distro = getUtility(IDistributionSet).getByName('ubuntu')
> + >>> de_archive_mirror = factory.makeMirror(ubuntu_distro,
> + ... "Technische Universitaet Dresden", country=82,
> + ... http_url="http://ubuntu.mirror.tudos.de/ubuntu/",
> + ... official_candidate=True)
> + >>> davis_station_archive = factory.makeMirror(ubuntu_distro,
> + ... "Davis Station", country=9,
> + ... http_url="http://mirror.davis.antarctica.org/ubuntu",
> + ... official_candidate=True)
> + >>> de_archive_mirror.status = MirrorStatus.OFFICIAL
> + >>> de_archive_prober_log = factory.makeMirrorProbeRecord(de_archive_mirror)
> + >>> logout()

Wrap the code at 78 characters.

...

> +Mirrors which are not official or do not have an HTTP URL may not be set as
> +country mirrors:
> +
> + >>> login('<email address hidden>')
> + >>> osuosl_mirror = factory.makeMirror(ubuntu_distro, "OSU Open Source Lab",
> + ... country=226,
> + ... ftp_url="ftp://ubuntu.osuosl.org/pub/ubuntu/",
> + ... official_candidate=True)
> + >>> osuosl_mirror.status = MirrorStatus.OFFICIAL
> + >>> print osuosl_mirror.http_base_url
> + None

Wrap the code at 78 characters.

> === modified file 'lib/lp/registry/interfaces/distribution.py'
> --- lib/lp/registry/interfaces/distribution.py 2010-03-24 21:59:58 +0000
> +++ lib/lp/registry/interfaces/distribution.py 2010-03-29 15:54:12 +0000
>
> @@ -321,6 +321,14 @@
> if it's not found.
> """
>
> + @operation_parameters(
> + country=copy_field(IDistributionMirror['country'], required=True),
> + mirror_type=copy_field(IDistributionMirror['content'], required=True))
> + @operation_returns_entry(IDistributionMirror)
> + @export_read_operation()
> + def getCountryMirror(country, mirror_type):
> + """Return the country DNS mirror for acountry and content type."""

grammar: s/acountry/a country/

> === modified file 'lib/lp/registry/interfaces/distributionmirror.py'
> --- lib/lp/registry/interfaces/distributionmirror.py 2010-02-22 15:50:06 +0000
> +++ lib/lp/registry/interfaces/distributionmirror.py 2010-03-29 15:54:12 +0000
> @@ -6,21 +6,24 @@
> __metaclass__ = type

 __all__ = [
...

> + 'CannotTransitionToCountryMirror',
> + 'CountryMirrorAlreadySet',
> + 'IDistributionMirror',
> + 'IMirrorDistroArchSeries',
> + 'IMirrorDistroSeriesSource',
> + 'IMirrorProbeRecord',
> + 'IDistributionMirrorSet',
> + 'IMirrorCDImageDistroSeries',
> + 'PROBE_INTERVAL',
> + 'MirrorContent',
> + 'MirrorFreshness',
> + 'MirrorHasNoHTTPUrl',
> + 'MirrorNotOfficial',
> + 'MirrorNotProbed',
> + 'MirrorSpeed',
> + 'MirrorStatus',
> + 'UnableToFetchCDImageFileList'

This is missing the PEP 8 trailing comma.

...

> +class MirrorHasNoHTTPUrl(CannotTransitionToCountryMirror):
> + """Distribution mirror has no HTTP URL.
> +
> + Raised when a user tries to make an official mirror a country mirror,
> + however the mirror has not HTTP URL set.
> + """

The capitalisation is inconsistent: MirrorHasNoHTTPURL.

> === modified file 'lib/lp/registry/stories/webservice/xx-distribution.txt'
> --- lib/lp/registry/stories/webservice/xx-distribution.txt 2010-02-23 17:36:27 +0000
> +++ lib/lp/registry/stories/webservice/xx-distribution.txt 2010-03-29 15:54:12 +0000

...

> +For "getCountryMirror", the mirror_type parameter must be "Archive" or
> +"CD Images":
> +
> + >>> uk_country_mirror_archive = webservice.named_get(
> + ... ubuntu['self_link'], 'getCountryMirror',
> + ... country=uk['self_link'],
> + ... mirror_type="Bogus")
> + >>> print uk_country_mirror_archive.jsonBody()
> + Traceback (most recent call last):
> + ...
> + ValueError: mirror_type: Invalid value "Bogus". Acceptable values are: Archive, CD Image

Wrap the code at 78 characters.

> === modified file 'lib/lp/testing/factory.py'
> --- lib/lp/testing/factory.py 2010-03-25 02:21:15 +0000
> +++ lib/lp/testing/factory.py 2010-03-29 15:54:12 +0000
> @@ -1981,8 +1981,22 @@
> team_list = self.makeMailingList(team, owner)
> return team, team_list
>
> + def makeMirrorProbeRecord(self, mirror):
> + """Create a probe record for a mirror of a distribution."""
> + log_file = StringIO()
> + log_file.write("Fake probe, nothing useful here.")
> + log_file.seek(0)
> +
> + library_alias = getUtility(ILibraryFileAliasSet).create(
> + name='foo', size=len(log_file.getvalue()),
> + file=log_file, contentType='text/plain')
> +
> + proberecord = mirror.newProbeRecord(library_alias)
> + return proberecord

lp/registry/tests/test_distributionmirror.py still uses
_create_probe_record() I think The test can use the factory method if you
make these changes:

from lp.testing import TestCaseWithFactory
...
class TestDistributionMirror(TestCaseWithFactory):
...
    def setUp():
        super(TestDistributionMirror, self).setUp()
...
    self.factory.makeMirrorProbeRecord(mirror)
...
    self.factory.makeMirrorProbeRecord(mirror)