Merge lp:~wgrant/launchpad/export-das-chroot into lp:launchpad

[William Grant]

Export DistroArchSeries chroot URLs on the webservice.

[William Grant (wgrant)]

This trivial branch adds and exports IDistroArchSeries.chroot_url. I think that just about says it all.

[Björn Tillenius (bjornt)]

Why not export getChroot() instead? It seems more useful to get the actual librarian file, rather than just a URL.

review: Needs Information

[William Grant (wgrant)]

On Sat, 2010-02-20 at 08:11 +0000, Björn Tillenius wrote:
> Review: Needs Information
> Why not export getChroot() instead? It seems more useful to get the actual librarian file, rather than just a URL.

ILFA isn't exported at the moment. All other places I can find seem to
just export a URL, and Julian suggested doing that on the bug. It would
be really nice to have attributes like hashes visible, though.

[Björn Tillenius (bjornt)]

On Sat, Feb 20, 2010 at 12:50:33AM -0000, William Grant wrote:
> === modified file 'lib/lp/soyuz/doc/distroarchseries.txt'
> --- lib/lp/soyuz/doc/distroarchseries.txt 2009-10-26 18:40:04 +0000
> +++ lib/lp/soyuz/doc/distroarchseries.txt 2010-02-20 00:50:31 +0000
> @@ -344,6 +344,14 @@
> >>> print_architectures(hoary.enabled_architectures)
> The Hoary Hedgehog Release for hppa (hppa) (ppa)
>
> +The architecture also has a 'chroot_url' attribute directly referencing
> +the file.
> +
> + >>> print hoary.getDistroArchSeries('hppa').chroot_url
> + http://localhost:58000/.../filename...
> + >>> print hoary.getDistroArchSeries('i386').chroot_url
> + None

When is chroot_url None? It'd be good to document this. Also, for test
correctness, I'd also (in addition to the current test) compare
chroot_url to getChroot().http_url, so that you know that you're not
returning just any URL.

> === modified file 'lib/lp/soyuz/interfaces/distroarchseries.py'
> --- lib/lp/soyuz/interfaces/distroarchseries.py 2009-07-17 00:26:05 +0000
> +++ lib/lp/soyuz/interfaces/distroarchseries.py 2010-02-20 00:50:31 +0000
> @@ -108,6 +108,12 @@
> Interface, # Really IArchive, circular import fixed below.
> title=_('Main Archive'),
> description=_("The main archive of the distroarchseries.")))
> + chroot_url = exported(
> + TextLine(
> + title=_("Build chroot URL"),
> + description=_(
> + "The URL to the current build chroot for this "
> + "distroarchseries.")))
>
> def updatePackageCount():
> """Update the cached binary package count for this distro arch
>
> === modified file 'lib/lp/soyuz/model/distroarchseries.py'
> --- lib/lp/soyuz/model/distroarchseries.py 2009-12-24 06:57:25 +0000
> +++ lib/lp/soyuz/model/distroarchseries.py 2010-02-20 00:50:31 +0000
> @@ -136,6 +136,13 @@
>
> return pocket_chroot.chroot
>
> + @property
> + def chroot_url(self):
> + chroot = self.getChroot()

This should have a """See `IDistroArchSeries`.""" docstring.

I'm approving this branch with these small comments resolved.

    vote approve

--
Björn Tillenius | https://launchpad.net/~bjornt

review: Approve

[Julian Edwards (julian-edwards)]

> Why not export getChroot() instead? It seems more useful to get the actual
> librarian file, rather than just a URL.

chroots are several hundred meg big - way too much to proxy through the webapp.

We return URLs as a matter of course for most objects now, Launchpad needs to be quicker, not slower when we tie up an app sever.

[Julian Edwards (julian-edwards)]

This bit should have some explanation text to say why this set up is needed. Otherwise, looks great, thanks for the fix William.

63 + >>> from zope.component import getUtility
64 + >>> from lp.registry.interfaces.distribution import IDistributionSet
65 +
66 + >>> login('<email address hidden>')
67 + >>> hoary = getUtility(IDistributionSet)['ubuntu'].getSeries('hoary')
68 + >>> chroot = factory.makeLibraryFileAlias()
69 + >>> unused = hoary.getDistroArchSeries('i386').addOrUpdateChroot(chroot)
70 + >>> logout()
71 +

review: Approve

[Björn Tillenius (bjornt)]

On Mon, Feb 22, 2010 at 10:27:17AM -0000, Julian Edwards wrote:
> > Why not export getChroot() instead? It seems more useful to get the actual
> > librarian file, rather than just a URL.
>
> chroots are several hundred meg big - way too much to proxy through the webapp.

Why does this matter if ILibraryFileAlias is exported, exposing meta
data, including http_url?

--
Björn Tillenius | https://launchpad.net/~bjornt

[Julian Edwards (julian-edwards)]

> On Mon, Feb 22, 2010 at 10:27:17AM -0000, Julian Edwards wrote:
> > > Why not export getChroot() instead? It seems more useful to get the actual
> > > librarian file, rather than just a URL.
> >
> > chroots are several hundred meg big - way too much to proxy through the
> webapp.
>
> Why does this matter if ILibraryFileAlias is exported, exposing meta
> data, including http_url?

Oh I see, expose the LFA. Yeah that would be ok, but I'd do it in addition to the other URL so that it's not yet another traversal to get the data you need. And LFA is not currently exported of course :)

[Francis J. Lacoste (flacoste)]

On February 22, 2010, Julian Edwards wrote:
> > On Mon, Feb 22, 2010 at 10:27:17AM -0000, Julian Edwards wrote:
> > > > Why not export getChroot() instead? It seems more useful to get the
> > > > actual librarian file, rather than just a URL.
> > >
> > > chroots are several hundred meg big - way too much to proxy through the
> >
> > webapp.
> >
> > Why does this matter if ILibraryFileAlias is exported, exposing meta
> > data, including http_url?
>
> Oh I see, expose the LFA. Yeah that would be ok, but I'd do it in addition
> to the other URL so that it's not yet another traversal to get the data
> you need. And LFA is not currently exported of course :)

We don't want to export LibraryFileAlias directly. These are mapped to
HostedFile in the web service implementation.

  reviewer leonardr

Leonard, could you give us some pointers on how this should be mapped to a
HostedFile.

Thanks

--
Francis J. Lacoste
<email address hidden>

[William Grant (wgrant)]

So, this was discussed a bit on IRC, but no real consensus was reached. Should I change it to export an IBytes, or should I be consistent with most of the rest of the application and export a librarian URL until we can export LFA more sanely?

[Julian Edwards (julian-edwards)]

> So, this was discussed a bit on IRC, but no real consensus was reached. Should
> I change it to export an IBytes, or should I be consistent with most of the
> rest of the application and export a librarian URL until we can export LFA
> more sanely?

I would be consistent and land the change that exports the URL. I'll land it for you today.

=== modified file 'lib/lp/soyuz/doc/distroarchseries.txt'
--- lib/lp/soyuz/doc/distroarchseries.txt	2009-10-26 18:40:04 +0000
+++ lib/lp/soyuz/doc/distroarchseries.txt	2010-02-22 10:48:27 +0000
@@ -344,6 +344,20 @@
     >>> print_architectures(hoary.enabled_architectures)
     The Hoary Hedgehog Release for hppa (hppa) (ppa)
 
+The architecture also has a 'chroot_url' attribute directly referencing
+the file.
+
+    >>> print hoary.getDistroArchSeries('hppa').chroot_url
+    http://localhost:58000/.../filename...
+    >>> hoary.getDistroArchSeries('hppa').chroot_url == \
+    ...     chroot.http_url
+    True
+
+If there is no chroot, chroot_url will be None.
+
+    >>> print hoary.getDistroArchSeries('i386').chroot_url
+    None
+
 `DistroSeries.enabled_architectures` results are ordered
 alphabetically by 'architecturetag'.
 

=== modified file 'lib/lp/soyuz/interfaces/distroarchseries.py'
--- lib/lp/soyuz/interfaces/distroarchseries.py	2009-07-17 00:26:05 +0000
+++ lib/lp/soyuz/interfaces/distroarchseries.py	2010-02-22 10:48:27 +0000
@@ -108,6 +108,13 @@
             Interface, # Really IArchive, circular import fixed below.
             title=_('Main Archive'),
             description=_("The main archive of the distroarchseries.")))
+    chroot_url = exported(
+        TextLine(
+            title=_("Build chroot URL"),
+            description=_(
+                "The URL to the current build chroot for this "
+                "distroarchseries."),
+            readonly=True))
 
     def updatePackageCount():
         """Update the cached binary package count for this distro arch

=== modified file 'lib/lp/soyuz/model/distroarchseries.py'
--- lib/lp/soyuz/model/distroarchseries.py	2009-12-24 06:57:25 +0000
+++ lib/lp/soyuz/model/distroarchseries.py	2010-02-22 10:48:27 +0000
@@ -136,6 +136,14 @@
 
         return pocket_chroot.chroot
 
+    @property
+    def chroot_url(self):
+        """See `IDistroArchSeries`."""
+        chroot = self.getChroot()
+        if chroot is None:
+            return None
+        return chroot.http_url
+
     def addOrUpdateChroot(self, chroot):
         """See `IDistroArchSeries`."""
         pocket_chroot = self.getPocketChroot()

=== renamed file 'lib/canonical/launchpad/pagetests/webservice/xx-distroarchseries.txt' => 'lib/lp/soyuz/stories/webservice/xx-distroarchseries.txt'
--- lib/canonical/launchpad/pagetests/webservice/xx-distroarchseries.txt	2009-08-13 15:12:16 +0000
+++ lib/lp/soyuz/stories/webservice/xx-distroarchseries.txt	2010-02-22 10:48:27 +0000
@@ -13,6 +13,18 @@
     >>> print current_series['self_link']
     http://.../ubuntu/hoary
 
+We'll first set up a buildd chroot, so we can check that its URL is
+exposed.
+
+    >>> from zope.component import getUtility
+    >>> from lp.registry.interfaces.distribution import IDistributionSet
+
+    >>> login('[e-mail]')
+    >>> hoary = getUtility(IDistributionSet)['ubuntu'].getSeries('hoary')
+    >>> chroot = factory.makeLibraryFileAlias()
+    >>> unused = hoary.getDistroArchSeries('i386').addOrUpdateChroot(chroot)
+    >>> logout()
+
     >>> distroarchseries = webservice.named_get(
     ...     current_series['self_link'], 'getDistroArchSeries',
     ...     archtag='i386').jsonBody()
@@ -22,6 +34,7 @@
     >>> from lazr.restful.testing.webservice import pprint_entry
     >>> pprint_entry(distroarchseries)
     architecture_tag: u'i386'
+    chroot_url: u'http://localhost:58000/.../filename...'
     display_name: u'Ubuntu Hoary i386'
     distroseries_link: u'http://.../ubuntu/hoary'
     is_nominated_arch_indep: True