Merge lp:~michael.nelson/launchpad/510331-syncsources-latest-pub into lp:launchpad
- 510331-syncsources-latest-pub
- Merge into devel
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Michael Nelson | ||||
Approved revision: | not available | ||||
Merged at revision: | not available | ||||
Proposed branch: | lp:~michael.nelson/launchpad/510331-syncsources-latest-pub | ||||
Merge into: | lp:launchpad | ||||
Diff against target: |
190 lines (+90/-22) 5 files modified
lib/lp/soyuz/doc/archive.txt (+11/-7) lib/lp/soyuz/interfaces/archive.py (+2/-2) lib/lp/soyuz/model/archive.py (+24/-11) lib/lp/soyuz/stories/webservice/xx-archive.txt (+9/-2) lib/lp/soyuz/tests/test_archive.py (+44/-0) |
||||
To merge this branch: | bzr merge lp:~michael.nelson/launchpad/510331-syncsources-latest-pub | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Graham Binns (community) | code | Approve | |
Review via email: mp+18070@code.launchpad.net |
Commit message
Ensures that syncSources only chooses the latest *PUBLISHED* sources, rather than simply the latest sources.
Description of the change
Michael Nelson (michael.nelson) wrote : | # |
Graham Binns (gmb) wrote : | # |
Hi Michael,
This looks good to me. One minor nitpick, but otherwise it's fine to
land as-is.
> === modified file 'lib/lp/
> --- lib/lp/
> +++ lib/lp/
> @@ -1121,6 +1112,23 @@
>
> self._copySourc
>
> + def _collectLatestP
> + """Private helper to collect the latest published sources for an
> + archive.
> + """
> + sources = []
> + for name in source_names:
> + # Check to see if the source package exists, and raise a useful
> + # error if it doesn't.
What qualifies as "A useful error" here? It'd be nice to say in the
comment - or better still in the docstring - "will raise a FooBarError
if the source package doesn't exist."
> + getUtility(
> + # Grabbing the item at index 0 ensures it's the most recent
> + # publication.
> + sources.append(
> + from_archive.
> + name=name, exact_match=True,
> + status=
> + return sources
> +
> def _copySources(self, sources, to_pocket, to_series=None,
> include_
> """Private helper function to copy sources to this archive.
>
Michael Nelson (michael.nelson) wrote : | # |
<noodles775> gmb: great. Did you have any thoughts about a better way to test the private method (as mentioned in the MP)?
<gmb> noodles775: Argh, must've skipped that bit of the mp. Sorry. Hang on...
<gmb> noodles775: So, no, I can't think of a better way (which is why I never balked at the use of rSP()). The only way I could think of is to test it indirectly, but I'd rather have the direct test myself.
<noodles775> Great, thanks for that.
Michael Nelson (michael.nelson) wrote : | # |
Hi gmb,
I just got back to this branch, tested it, and had errors or the webservice branch that tested syncSource(s) through the API. The error was simply because the sources used in the test weren't in the published status, but it was 500ing, rather than a nice API error, so I've updated the code to avoid that also.
Here's the diff:
=== modified file 'lib/lp/
--- lib/lp/
+++ lib/lp/
@@ -1127,10 +1127,11 @@
# Grabbing the item at index 0 ensures it's the most recent
# publication.
- sources.append(
- from_archive.
- name=name, exact_match=True,
- status=
+ published_sources = from_archive.
+ name=name, exact_match=True,
+ status=
+ if published_
+ sources.
return sources
def _copySources(self, sources, to_pocket, to_series=None,
=== modified file 'lib/lp/
--- lib/lp/
+++ lib/lp/
@@ -576,9 +576,12 @@
... sourcename=
... archive=
+ >>> from lp.soyuz.
+ ... PackagePublishi
>>> ignore = test_publisher.
... sourcename=
- ... archive=
+ ... archive=
+ ... status=
>>> ignore = test_publisher.
... sourcename=
@@ -742,6 +745,8 @@
>>> private_publication = test_publisher.
... cprov.archive, 'foocomm', '1.0-1')
+ >>> private_
+ ... PackagePublishi
>>> logout()
@@ -908,8 +913,10 @@
version.
>>> login('<email address hidden>')
- >>> unused = test_publisher.
+ >>> subsequent_version = test_publisher.
... cprov.archive, 'foocomm', '1.0-2')
+ >>> subsequent_
+ ... PackagePublishi
>>> logout()
>>> print cprov_webservic
Graham Binns (gmb) wrote : | # |
As discussed on IRC, you should test for `not foo.is_empty()` rather than `foo.count() > 0` as it's more efficient. Other than that, this change looks fine.
Michael Nelson (michael.nelson) wrote : | # |
<noodles775> gmb, jtv: http://
<noodles775> Looks like is_empty isn't included on the ISQLObjectResultSet in storm.zope.
<gmb> ?!
<gmb> Oh, ah.
<gmb> I see.
<gmb> That's suboptimal.
<gmb> noodles775: In that case, just go back to using .count(). You could maybe file a tech-debt bug about changing getPublishedSou
<noodles775> Yep, thanks.
Preview Diff
1 | === modified file 'lib/lp/soyuz/doc/archive.txt' | |||
2 | --- lib/lp/soyuz/doc/archive.txt 2010-01-12 08:59:23 +0000 | |||
3 | +++ lib/lp/soyuz/doc/archive.txt 2010-02-02 12:06:20 +0000 | |||
4 | @@ -2349,13 +2349,17 @@ | |||
5 | 2349 | >>> test_publisher.addFakeChroots(hoary) | 2349 | >>> test_publisher.addFakeChroots(hoary) |
6 | 2350 | >>> unused = test_publisher.setUpDefaultDistroSeries(hoary) | 2350 | >>> unused = test_publisher.setUpDefaultDistroSeries(hoary) |
7 | 2351 | >>> discard = test_publisher.getPubSource( | 2351 | >>> discard = test_publisher.getPubSource( |
15 | 2352 | ... sourcename="package1", version="1.0", archive=cprov.archive) | 2352 | ... sourcename="package1", version="1.0", archive=cprov.archive, |
16 | 2353 | >>> discard = test_publisher.getPubSource( | 2353 | ... status=PackagePublishingStatus.PUBLISHED) |
17 | 2354 | ... sourcename="package1", version="1.1", archive=cprov.archive) | 2354 | >>> discard = test_publisher.getPubSource( |
18 | 2355 | >>> discard = test_publisher.getPubSource( | 2355 | ... sourcename="package1", version="1.1", archive=cprov.archive, |
19 | 2356 | ... sourcename="package2", version="1.0", archive=cprov.archive) | 2356 | ... status=PackagePublishingStatus.PUBLISHED) |
20 | 2357 | >>> discard = test_publisher.getPubSource( | 2357 | >>> discard = test_publisher.getPubSource( |
21 | 2358 | ... sourcename="pack", version="1.0", archive=cprov.archive) | 2358 | ... sourcename="package2", version="1.0", archive=cprov.archive, |
22 | 2359 | ... status=PackagePublishingStatus.PUBLISHED) | ||
23 | 2360 | >>> discard = test_publisher.getPubSource( | ||
24 | 2361 | ... sourcename="pack", version="1.0", archive=cprov.archive, | ||
25 | 2362 | ... status=PackagePublishingStatus.PUBLISHED) | ||
26 | 2359 | 2363 | ||
27 | 2360 | Now we have package1 1.0 and 1.1, and package2 1.0 in cprov's PPA. We | 2364 | Now we have package1 1.0 and 1.1, and package2 1.0 in cprov's PPA. We |
28 | 2361 | can ask syncSources to synchronise these packages into mark's PPA in the | 2365 | can ask syncSources to synchronise these packages into mark's PPA in the |
29 | 2362 | 2366 | ||
30 | === modified file 'lib/lp/soyuz/interfaces/archive.py' | |||
31 | --- lib/lp/soyuz/interfaces/archive.py 2010-01-12 03:45:02 +0000 | |||
32 | +++ lib/lp/soyuz/interfaces/archive.py 2010-02-02 12:06:20 +0000 | |||
33 | @@ -896,8 +896,8 @@ | |||
34 | 896 | to_series=None, include_binaries=False): | 896 | to_series=None, include_binaries=False): |
35 | 897 | """Synchronise (copy) named sources into this archive from another. | 897 | """Synchronise (copy) named sources into this archive from another. |
36 | 898 | 898 | ||
39 | 899 | It will copy the most recent versions of the named sources to | 899 | It will copy the most recent PUBLISHED versions of the named |
40 | 900 | the destination archive if necessary. | 900 | sources to the destination archive if necessary. |
41 | 901 | 901 | ||
42 | 902 | This operation will only succeeds when all requested packages | 902 | This operation will only succeeds when all requested packages |
43 | 903 | are synchronised between the archives. If any of the requested | 903 | are synchronised between the archives. If any of the requested |
44 | 904 | 904 | ||
45 | === modified file 'lib/lp/soyuz/model/archive.py' | |||
46 | --- lib/lp/soyuz/model/archive.py 2010-01-12 08:31:42 +0000 | |||
47 | +++ lib/lp/soyuz/model/archive.py 2010-02-02 12:06:20 +0000 | |||
48 | @@ -1096,17 +1096,8 @@ | |||
49 | 1096 | to_series=None, include_binaries=False): | 1096 | to_series=None, include_binaries=False): |
50 | 1097 | """See `IArchive`.""" | 1097 | """See `IArchive`.""" |
51 | 1098 | # Find and validate the source package names in source_names. | 1098 | # Find and validate the source package names in source_names. |
63 | 1099 | sources = [] | 1099 | sources = self._collectLatestPublishedSources( |
64 | 1100 | for name in source_names: | 1100 | from_archive, source_names) |
54 | 1101 | # Check to see if the source package exists, and raise a useful | ||
55 | 1102 | # error if it doesn't. | ||
56 | 1103 | getUtility(ISourcePackageNameSet)[name] | ||
57 | 1104 | # Grabbing the item at index 0 ensures it's the most recent | ||
58 | 1105 | # publication. | ||
59 | 1106 | sources.append( | ||
60 | 1107 | from_archive.getPublishedSources( | ||
61 | 1108 | name=name, exact_match=True)[0]) | ||
62 | 1109 | |||
65 | 1110 | self._copySources(sources, to_pocket, to_series, include_binaries) | 1101 | self._copySources(sources, to_pocket, to_series, include_binaries) |
66 | 1111 | 1102 | ||
67 | 1112 | def syncSource(self, source_name, version, from_archive, to_pocket, | 1103 | def syncSource(self, source_name, version, from_archive, to_pocket, |
68 | @@ -1121,6 +1112,28 @@ | |||
69 | 1121 | 1112 | ||
70 | 1122 | self._copySources([source], to_pocket, to_series, include_binaries) | 1113 | self._copySources([source], to_pocket, to_series, include_binaries) |
71 | 1123 | 1114 | ||
72 | 1115 | def _collectLatestPublishedSources(self, from_archive, source_names): | ||
73 | 1116 | """Private helper to collect the latest published sources for an | ||
74 | 1117 | archive. | ||
75 | 1118 | |||
76 | 1119 | :raises NoSuchSourcePackageName: If any of the source_names do not | ||
77 | 1120 | exist. | ||
78 | 1121 | """ | ||
79 | 1122 | sources = [] | ||
80 | 1123 | for name in source_names: | ||
81 | 1124 | # Check to see if the source package exists. This will raise | ||
82 | 1125 | # a NoSuchSourcePackageName exception if the source package | ||
83 | 1126 | # doesn't exist. | ||
84 | 1127 | getUtility(ISourcePackageNameSet)[name] | ||
85 | 1128 | # Grabbing the item at index 0 ensures it's the most recent | ||
86 | 1129 | # publication. | ||
87 | 1130 | published_sources = from_archive.getPublishedSources( | ||
88 | 1131 | name=name, exact_match=True, | ||
89 | 1132 | status=PackagePublishingStatus.PUBLISHED) | ||
90 | 1133 | if published_sources.count() > 0: | ||
91 | 1134 | sources.append(published_sources[0]) | ||
92 | 1135 | return sources | ||
93 | 1136 | |||
94 | 1124 | def _copySources(self, sources, to_pocket, to_series=None, | 1137 | def _copySources(self, sources, to_pocket, to_series=None, |
95 | 1125 | include_binaries=False): | 1138 | include_binaries=False): |
96 | 1126 | """Private helper function to copy sources to this archive. | 1139 | """Private helper function to copy sources to this archive. |
97 | 1127 | 1140 | ||
98 | === modified file 'lib/lp/soyuz/stories/webservice/xx-archive.txt' | |||
99 | --- lib/lp/soyuz/stories/webservice/xx-archive.txt 2009-10-07 08:28:03 +0000 | |||
100 | +++ lib/lp/soyuz/stories/webservice/xx-archive.txt 2010-02-02 12:06:20 +0000 | |||
101 | @@ -576,9 +576,12 @@ | |||
102 | 576 | ... sourcename="package1", version="1.0", | 576 | ... sourcename="package1", version="1.0", |
103 | 577 | ... archive=ubuntu_db.main_archive) | 577 | ... archive=ubuntu_db.main_archive) |
104 | 578 | 578 | ||
105 | 579 | >>> from lp.soyuz.interfaces.publishing import ( | ||
106 | 580 | ... PackagePublishingStatus) | ||
107 | 579 | >>> ignore = test_publisher.getPubSource( | 581 | >>> ignore = test_publisher.getPubSource( |
108 | 580 | ... sourcename="package1", version="1.1", | 582 | ... sourcename="package1", version="1.1", |
110 | 581 | ... archive=ubuntu_db.main_archive) | 583 | ... archive=ubuntu_db.main_archive, |
111 | 584 | ... status=PackagePublishingStatus.PUBLISHED) | ||
112 | 582 | 585 | ||
113 | 583 | >>> ignore = test_publisher.getPubSource( | 586 | >>> ignore = test_publisher.getPubSource( |
114 | 584 | ... sourcename="package2", version="1.0", | 587 | ... sourcename="package2", version="1.0", |
115 | @@ -742,6 +745,8 @@ | |||
116 | 742 | 745 | ||
117 | 743 | >>> private_publication = test_publisher.createSource( | 746 | >>> private_publication = test_publisher.createSource( |
118 | 744 | ... cprov.archive, 'foocomm', '1.0-1') | 747 | ... cprov.archive, 'foocomm', '1.0-1') |
119 | 748 | >>> private_publication.secure_record.status = ( | ||
120 | 749 | ... PackagePublishingStatus.PUBLISHED) | ||
121 | 745 | 750 | ||
122 | 746 | >>> logout() | 751 | >>> logout() |
123 | 747 | 752 | ||
124 | @@ -908,8 +913,10 @@ | |||
125 | 908 | version. | 913 | version. |
126 | 909 | 914 | ||
127 | 910 | >>> login('foo.bar@canonical.com') | 915 | >>> login('foo.bar@canonical.com') |
129 | 911 | >>> unused = test_publisher.createSource( | 916 | >>> subsequent_version = test_publisher.createSource( |
130 | 912 | ... cprov.archive, 'foocomm', '1.0-2') | 917 | ... cprov.archive, 'foocomm', '1.0-2') |
131 | 918 | >>> subsequent_version.secure_record.status = ( | ||
132 | 919 | ... PackagePublishingStatus.PUBLISHED) | ||
133 | 913 | >>> logout() | 920 | >>> logout() |
134 | 914 | 921 | ||
135 | 915 | >>> print cprov_webservice.named_post( | 922 | >>> print cprov_webservice.named_post( |
136 | 916 | 923 | ||
137 | === modified file 'lib/lp/soyuz/tests/test_archive.py' | |||
138 | --- lib/lp/soyuz/tests/test_archive.py 2010-01-12 04:21:42 +0000 | |||
139 | +++ lib/lp/soyuz/tests/test_archive.py 2010-02-02 12:06:20 +0000 | |||
140 | @@ -513,6 +513,50 @@ | |||
141 | 513 | self.archive.disable() | 513 | self.archive.disable() |
142 | 514 | self.assertRaises(AssertionError, self.archive.disable) | 514 | self.assertRaises(AssertionError, self.archive.disable) |
143 | 515 | 515 | ||
144 | 516 | class TestCollectLatestPublishedSources(TestCaseWithFactory): | ||
145 | 517 | """Ensure that the private helper method works as expected.""" | ||
146 | 518 | |||
147 | 519 | layer = LaunchpadZopelessLayer | ||
148 | 520 | |||
149 | 521 | def setUp(self): | ||
150 | 522 | """Setup an archive with relevant publications.""" | ||
151 | 523 | super(TestCollectLatestPublishedSources, self).setUp() | ||
152 | 524 | self.publisher = SoyuzTestPublisher() | ||
153 | 525 | self.publisher.prepareBreezyAutotest() | ||
154 | 526 | |||
155 | 527 | # Create an archive with some published sources. We'll store | ||
156 | 528 | # a reference to the naked archive so that we can call | ||
157 | 529 | # the private method which is not defined on the interface. | ||
158 | 530 | self.archive = self.factory.makeArchive() | ||
159 | 531 | self.naked_archive = removeSecurityProxy(self.archive) | ||
160 | 532 | |||
161 | 533 | self.pub_1 = self.publisher.getPubSource( | ||
162 | 534 | version='0.5.11~ppa1', archive=self.archive, sourcename="foo", | ||
163 | 535 | status=PackagePublishingStatus.PUBLISHED) | ||
164 | 536 | |||
165 | 537 | self.pub_2 = self.publisher.getPubSource( | ||
166 | 538 | version='0.5.11~ppa2', archive=self.archive, sourcename="foo", | ||
167 | 539 | status=PackagePublishingStatus.PUBLISHED) | ||
168 | 540 | |||
169 | 541 | self.pub_3 = self.publisher.getPubSource( | ||
170 | 542 | version='0.9', archive=self.archive, sourcename="bar", | ||
171 | 543 | status=PackagePublishingStatus.PUBLISHED) | ||
172 | 544 | |||
173 | 545 | def test_collectLatestPublishedSources_returns_latest(self): | ||
174 | 546 | pubs = self.naked_archive._collectLatestPublishedSources( | ||
175 | 547 | self.archive, ["foo"]) | ||
176 | 548 | self.assertEqual(1, len(pubs)) | ||
177 | 549 | self.assertEqual('0.5.11~ppa2', pubs[0].source_package_version) | ||
178 | 550 | |||
179 | 551 | def test_collectLatestPublishedSources_returns_published_only(self): | ||
180 | 552 | # Set the status of the latest pub to DELETED and ensure that it | ||
181 | 553 | # is not returned. | ||
182 | 554 | self.pub_2.secure_record.status = PackagePublishingStatus.DELETED | ||
183 | 555 | |||
184 | 556 | pubs = self.naked_archive._collectLatestPublishedSources( | ||
185 | 557 | self.archive, ["foo"]) | ||
186 | 558 | self.assertEqual(1, len(pubs)) | ||
187 | 559 | self.assertEqual('0.5.11~ppa1', pubs[0].source_package_version) | ||
188 | 516 | 560 | ||
189 | 517 | def test_suite(): | 561 | def test_suite(): |
190 | 518 | return unittest.TestLoader().loadTestsFromName(__name__) | 562 | return unittest.TestLoader().loadTestsFromName(__name__) |
= Summary =
Fixes 510331 by ensuring that syncSources only finds pub records that are currently PUBLISHED.
== Proposed fix ==
== Pre-implementation notes ==
See note on the bug, as well as: syncSource( ) already allows you to specify the version... but with the solution you've suggested for syncSources()
<noodles> ok, sounds good. Also with bug 510331
<bigjools> noodles: yeah that one's easy
<noodles> IArchive.
<bigjools> ?
<noodles> it would mean that we'd not copy deleted sources even when there is no published one... is that expected?
<bigjools> yes, I think so
<noodles> OK. easy then.
<bigjools> well, would you expect it to?
<noodles> I'm imagining using the web ui, looking at a bunch of deleted sources and selecting them...
<bigjools> that's syncSource behaviour really
<bigjools> syncSources is more automatic
<bigjools> but we can make it explicit in the docstring
<noodles> OK.
== Implementation details ==
I wasn't sure of the best way to test the private helper method. I ended up going with removeSecurityP roxy() for the test, but would be happy to hear about a neater solution?
== Tests == stPublishedSour ces
bin/test -vvt doc/archive.txt -t TestCollectLate
== Demo and Q/A ==
We can Q/A this using the API on dogfood (or edge).
= Launchpad lint =
Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.
Linting changed files: soyuz/doc/ archive. txt soyuz/tests/ test_archive. py soyuz/model/ archive. py
lib/lp/
lib/lp/
lib/lp/
== Pylint notices ==
lib/lp/ soyuz/model/ archive. py .event' (No module named lifecycle)
1099: [C0301] Line too long (81/78) -- fixed.
14: [F0401] Unable to import 'lazr.lifecycle