Launchpad itself

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

  1. export-das-chroot
  2. Merge into devel
Proposed by William Grant
Status: Merged
Approved by: Julian Edwards
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~wgrant/launchpad/export-das-chroot
Merge into: lp:launchpad
Diff against target: 92 lines (+42/-0)
4 files modified
lib/lp/soyuz/doc/distroarchseries.txt (+14/-0)
lib/lp/soyuz/interfaces/distroarchseries.py (+7/-0)
lib/lp/soyuz/model/distroarchseries.py (+8/-0)
lib/lp/soyuz/stories/webservice/xx-distroarchseries.txt (+13/-0)
To merge this branch: bzr merge lp:~wgrant/launchpad/export-das-chroot
Reviewer Review Type Date Requested Status
Julian Edwards (community) Approve
Björn Tillenius (community) Approve
Leonard Richardson Pending
Review via email: mp+19759@code.launchpad.net

Commit message

Export DistroArchSeries chroot URLs on the webservice.

To post a comment you must log in.
Revision history for this message
William Grant (wgrant) wrote :

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

Revision history for this message
Björn Tillenius (bjornt) wrote :

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

review: Needs Information
Revision history for this message
William Grant (wgrant) wrote :

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.

Revision history for this message
Björn Tillenius (bjornt) wrote :

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
Revision history for this message
Julian Edwards (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.

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.

Revision history for this message
Julian Edwards (julian-edwards) wrote :

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
Revision history for this message
Björn Tillenius (bjornt) 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?

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

Revision history for this message
Julian Edwards (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 :)

Revision history for this message
Francis J. Lacoste (flacoste) wrote :

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>

Revision history for this message
William Grant (wgrant) wrote :

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?

Revision history for this message
Julian Edwards (julian-edwards) wrote :

> 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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
344 >>> print_architectures(hoary.enabled_architectures)344 >>> print_architectures(hoary.enabled_architectures)
345 The Hoary Hedgehog Release for hppa (hppa) (ppa)345 The Hoary Hedgehog Release for hppa (hppa) (ppa)
346346
347The architecture also has a 'chroot_url' attribute directly referencing
348the file.
349
350 >>> print hoary.getDistroArchSeries('hppa').chroot_url
351 http://localhost:58000/.../filename...
352 >>> hoary.getDistroArchSeries('hppa').chroot_url == \
353 ... chroot.http_url
354 True
355
356If there is no chroot, chroot_url will be None.
357
358 >>> print hoary.getDistroArchSeries('i386').chroot_url
359 None
360
347`DistroSeries.enabled_architectures` results are ordered361`DistroSeries.enabled_architectures` results are ordered
348alphabetically by 'architecturetag'.362alphabetically by 'architecturetag'.
349363
350364
=== 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 @@
108 Interface, # Really IArchive, circular import fixed below.108 Interface, # Really IArchive, circular import fixed below.
109 title=_('Main Archive'),109 title=_('Main Archive'),
110 description=_("The main archive of the distroarchseries.")))110 description=_("The main archive of the distroarchseries.")))
111 chroot_url = exported(
112 TextLine(
113 title=_("Build chroot URL"),
114 description=_(
115 "The URL to the current build chroot for this "
116 "distroarchseries."),
117 readonly=True))
111118
112 def updatePackageCount():119 def updatePackageCount():
113 """Update the cached binary package count for this distro arch120 """Update the cached binary package count for this distro arch
114121
=== 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 @@
136136
137 return pocket_chroot.chroot137 return pocket_chroot.chroot
138138
139 @property
140 def chroot_url(self):
141 """See `IDistroArchSeries`."""
142 chroot = self.getChroot()
143 if chroot is None:
144 return None
145 return chroot.http_url
146
139 def addOrUpdateChroot(self, chroot):147 def addOrUpdateChroot(self, chroot):
140 """See `IDistroArchSeries`."""148 """See `IDistroArchSeries`."""
141 pocket_chroot = self.getPocketChroot()149 pocket_chroot = self.getPocketChroot()
142150
=== 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 @@
13 >>> print current_series['self_link']13 >>> print current_series['self_link']
14 http://.../ubuntu/hoary14 http://.../ubuntu/hoary
1515
16We'll first set up a buildd chroot, so we can check that its URL is
17exposed.
18
19 >>> from zope.component import getUtility
20 >>> from lp.registry.interfaces.distribution import IDistributionSet
21
22 >>> login('foo.bar@canonical.com')
23 >>> hoary = getUtility(IDistributionSet)['ubuntu'].getSeries('hoary')
24 >>> chroot = factory.makeLibraryFileAlias()
25 >>> unused = hoary.getDistroArchSeries('i386').addOrUpdateChroot(chroot)
26 >>> logout()
27
16 >>> distroarchseries = webservice.named_get(28 >>> distroarchseries = webservice.named_get(
17 ... current_series['self_link'], 'getDistroArchSeries',29 ... current_series['self_link'], 'getDistroArchSeries',
18 ... archtag='i386').jsonBody()30 ... archtag='i386').jsonBody()
@@ -22,6 +34,7 @@
22 >>> from lazr.restful.testing.webservice import pprint_entry34 >>> from lazr.restful.testing.webservice import pprint_entry
23 >>> pprint_entry(distroarchseries)35 >>> pprint_entry(distroarchseries)
24 architecture_tag: u'i386'36 architecture_tag: u'i386'
37 chroot_url: u'http://localhost:58000/.../filename...'
25 display_name: u'Ubuntu Hoary i386'38 display_name: u'Ubuntu Hoary i386'
26 distroseries_link: u'http://.../ubuntu/hoary'39 distroseries_link: u'http://.../ubuntu/hoary'
27 is_nominated_arch_indep: True40 is_nominated_arch_indep: True