Launchpad itself

Merge lp:~sinzui/launchpad/prf-files into lp:launchpad

  1. prf-files
  2. Merge into devel
Proposed by Curtis Hovey
Status: Merged
Merged at revision: not available
Proposed branch: lp:~sinzui/launchpad/prf-files
Merge into: lp:launchpad
Diff against target: 139 lines
2 files modified
lib/lp/registry/scripts/productreleasefinder/finder.py (+0/-9)
lib/lp/registry/tests/test_prf_finder.py (+38/-30)
To merge this branch: bzr merge lp:~sinzui/launchpad/prf-files
Reviewer Review Type Date Requested Status
Francis J. Lacoste (community) release-critical Approve
Aaron Bentley (community) code Approve
Canonical Launchpad Engineering rc code Pending
Review via email: mp+14312@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Curtis Hovey (sinzui) wrote :

This is my branch to ensure the PRF uploads all the files for a release.

    lp:~sinzui/launchpad/prf-files
    Diff size: 137
    Launchpad bug: https://bugs.launchpad.net/bugs/415595
    Test command: ./bin/test -vvt "test_prf_finder"
    Pre-implementation: no one
    Target release: 3.1.10

= Product release finder should import all files =

The product release finder should import all files for a particular release.
For example, I have sushi-1.0.1.tar.bz2 and sushi-1.0.1.tar.gz, but the
release finder only imports the .tar.bz2.

Testing reveals that this is not fixed yet. Running the PRF in debug mode I
can see an abort is called during addReleaseTarball(). The code has a loop
that duplicates the old hasReleaseFile() method to end early if there is a
binary associated with the release.

== Rules ==

We want to remove the whole loop because hasReleaseFile() makes the decision;
addReleaseTarball() should do as it is told. This will also make adding files
faster since the the loops not needed.

== QA ==

Run the product release finder.
Visit the a the 1.0.0 release of the /sushi.nigiri project
    * Verify that the release has both:
      sushi-1.0.0.tar.gz
      sushi-1.0.0.tar.bz2

Any project can be test in any environment by setting the target series'
release URL pattern to http://sushi.ikkoku.de/downloads/1.0/*

== Lint ==

Linting changed files:
  lib/lp/registry/scripts/productreleasefinder/finder.py
  lib/lp/registry/tests/test_prf_finder.py

== Test ==

    * lib/lp/registry/tests/test_prf_finder.py
      * Added a test to verify that multiple release tarball can be uploaded.
      * Extracted the duplicate test tarball file code to a helper.

== Implementation ==

    * lib/lp/registry/scripts/productreleasefinder/finder.py
      * Deleted the loop that aborts adding a second tarball for a release.
        already has binary.

Revision history for this message
Aaron Bentley (abentley) wrote :

This looks fine, except line 45 of the patch, which looks like it shouldn't be there.

review: Approve (code)
Revision history for this message
Curtis Hovey (sinzui) wrote :

> This looks fine, except line 45 of the patch, which looks like it shouldn't be
> there.

I Removed the extra line. PEP8.py also reported that another test was missing a blank line.

=== modified file 'lib/lp/registry/tests/test_prf_finder.py'
--- lib/lp/registry/tests/test_prf_finder.py 2009-11-02 17:35:34 +0000
+++ lib/lp/registry/tests/test_prf_finder.py 2009-11-02 19:42:51 +0000
@@ -102,6 +102,7 @@
     def test_handleProduct(self):
         # test that handleProduct() correctly calls handleRelease()
         class DummyProductReleaseFinder(ProductReleaseFinder):
+
             def __init__(self, ztm, log):
                 ProductReleaseFinder.__init__(self, ztm, log)
                 self.seen_releases = []
@@ -174,7 +175,6 @@
         shutil.rmtree(self.release_root, ignore_errors=True)
         reset_logging()

-
     def test_handleRelease(self):
         ztm = self.layer.txn
         logging.basicConfig(level=logging.CRITICAL)

Revision history for this message
Francis J. Lacoste (flacoste) :
review: Approve (release-critical)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/registry/scripts/productreleasefinder/finder.py'
--- lib/lp/registry/scripts/productreleasefinder/finder.py 2009-10-25 16:39:15 +0000
+++ lib/lp/registry/scripts/productreleasefinder/finder.py 2009-11-02 19:45:23 +0000
@@ -186,15 +186,6 @@
186 owner=product.owner, datereleased=datetime.now(pytz.UTC))186 owner=product.owner, datereleased=datetime.now(pytz.UTC))
187 self.log.info("Created new release %s for %s/%s",187 self.log.info("Created new release %s for %s/%s",
188 release_name, product_name, series_name)188 release_name, product_name, series_name)
189
190 # If we already have a code tarball, stop here.
191 for fileinfo in release.files:
192 if fileinfo.filetype == UpstreamFileType.CODETARBALL:
193 self.log.debug("%s/%s/%s already has a code tarball",
194 product_name, series_name, release_name)
195 self.ztm.abort()
196 return
197
198 release.addReleaseFile(189 release.addReleaseFile(
199 filename, file, content_type, uploader=product.owner)190 filename, file, content_type, uploader=product.owner)
200 self.ztm.commit()191 self.ztm.commit()
201192
=== modified file 'lib/lp/registry/tests/test_prf_finder.py'
--- lib/lp/registry/tests/test_prf_finder.py 2009-10-25 16:49:35 +0000
+++ lib/lp/registry/tests/test_prf_finder.py 2009-11-02 19:45:23 +0000
@@ -102,6 +102,7 @@
102 def test_handleProduct(self):102 def test_handleProduct(self):
103 # test that handleProduct() correctly calls handleRelease()103 # test that handleProduct() correctly calls handleRelease()
104 class DummyProductReleaseFinder(ProductReleaseFinder):104 class DummyProductReleaseFinder(ProductReleaseFinder):
105
105 def __init__(self, ztm, log):106 def __init__(self, ztm, log):
106 ProductReleaseFinder.__init__(self, ztm, log)107 ProductReleaseFinder.__init__(self, ztm, log)
107 self.seen_releases = []108 self.seen_releases = []
@@ -154,6 +155,16 @@
154155
155 layer = LaunchpadZopelessLayer156 layer = LaunchpadZopelessLayer
156157
158 def create_tarball(self, file_name):
159 """create a release tarball for testing"""
160 file_path = os.path.join(self.release_root, file_name)
161 try:
162 fp = open(file_path, 'w')
163 fp.write('foo')
164 finally:
165 fp.close()
166 return file_path, file_name
167
157 def setUp(self):168 def setUp(self):
158 LaunchpadZopelessLayer.switchDbUser(169 LaunchpadZopelessLayer.switchDbUser(
159 config.productreleasefinder.dbuser)170 config.productreleasefinder.dbuser)
@@ -168,14 +179,9 @@
168 ztm = self.layer.txn179 ztm = self.layer.txn
169 logging.basicConfig(level=logging.CRITICAL)180 logging.basicConfig(level=logging.CRITICAL)
170 prf = ProductReleaseFinder(ztm, logging.getLogger())181 prf = ProductReleaseFinder(ztm, logging.getLogger())
171 file_name = 'evolution-42.0.orig.tar.gz'
172 alt_file_name = 'evolution-42.0.orig.tar.bz2'182 alt_file_name = 'evolution-42.0.orig.tar.bz2'
173183 file_path, file_name = self.create_tarball(
174 # create a release tarball184 'evolution-42.0.orig.tar.gz')
175 fp = open(os.path.join(
176 self.release_root, file_name), 'w')
177 fp.write('foo')
178 fp.close()
179185
180 self.assertEqual(186 self.assertEqual(
181 prf.hasReleaseFile('evolution', 'trunk', '42.0', file_name),187 prf.hasReleaseFile('evolution', 'trunk', '42.0', file_name),
@@ -184,8 +190,7 @@
184 prf.hasReleaseFile('evolution', 'trunk', '42.0', alt_file_name),190 prf.hasReleaseFile('evolution', 'trunk', '42.0', alt_file_name),
185 False)191 False)
186192
187 prf.handleRelease(193 prf.handleRelease('evolution', 'trunk', file_path)
188 'evolution', 'trunk', self.release_url + '/' + file_name)
189194
190 self.assertEqual(195 self.assertEqual(
191 prf.hasReleaseFile('evolution', 'trunk', '42.0', file_name),196 prf.hasReleaseFile('evolution', 'trunk', '42.0', file_name),
@@ -234,15 +239,8 @@
234239
235 logging.basicConfig(level=logging.CRITICAL)240 logging.basicConfig(level=logging.CRITICAL)
236 prf = ProductReleaseFinder(ztm, logging.getLogger())241 prf = ProductReleaseFinder(ztm, logging.getLogger())
237242 file_path, file_name = self.create_tarball('evolution-2.1.6.tar.gz')
238 # create a release tarball243 prf.handleRelease('evolution', 'trunk', file_path)
239 fp = open(os.path.join(
240 self.release_root, 'evolution-2.1.6.tar.gz'), 'w')
241 fp.write('foo')
242 fp.close()
243
244 prf.handleRelease('evolution', 'trunk',
245 self.release_url + '/evolution-2.1.6.tar.gz')
246244
247 # verify that we now have files attached to the release:245 # verify that we now have files attached to the release:
248 evo = getUtility(IProductSet).getByName('evolution')246 evo = getUtility(IProductSet).getByName('evolution')
@@ -257,23 +255,33 @@
257 ztm = self.layer.txn255 ztm = self.layer.txn
258 logging.basicConfig(level=logging.CRITICAL)256 logging.basicConfig(level=logging.CRITICAL)
259 prf = ProductReleaseFinder(ztm, logging.getLogger())257 prf = ProductReleaseFinder(ztm, logging.getLogger())
260258 file_path, file_name = self.create_tarball('evolution-42.0.tar.gz')
261 # create a release tarball259 prf.handleRelease('evolution', 'trunk', file_path)
262 fp = open(os.path.join(260 prf.handleRelease('evolution', 'trunk', file_path)
263 self.release_root, 'evolution-42.0.tar.gz'), 'w')
264 fp.write('foo')
265 fp.close()
266
267 prf.handleRelease('evolution', 'trunk',
268 self.release_url + '/evolution-42.0.tar.gz')
269 prf.handleRelease('evolution', 'trunk',
270 self.release_url + '/evolution-42.0.tar.gz')
271
272 evo = getUtility(IProductSet).getByName('evolution')261 evo = getUtility(IProductSet).getByName('evolution')
273 trunk = evo.getSeries('trunk')262 trunk = evo.getSeries('trunk')
274 release = trunk.getRelease('42.0')263 release = trunk.getRelease('42.0')
275 self.assertEqual(release.files.count(), 1)264 self.assertEqual(release.files.count(), 1)
276265
266 def test_handleRelease_alternate_verstion(self):
267 """Verify that tar.gz and tar.bz2 versions are both uploaded."""
268 ztm = self.layer.txn
269 logging.basicConfig(level=logging.CRITICAL)
270 prf = ProductReleaseFinder(ztm, logging.getLogger())
271 file_path, file_name = self.create_tarball('evolution-45.0.tar.gz')
272 alt_file_path, alt_file_name = self.create_tarball(
273 'evolution-45.0.tar.bz2')
274 prf.handleRelease('evolution', 'trunk', file_path)
275 prf.handleRelease('evolution', 'trunk', alt_file_path)
276 evo = getUtility(IProductSet).getByName('evolution')
277 trunk = evo.getSeries('trunk')
278 release = trunk.getRelease('45.0')
279 release_filenames = [file_info.libraryfile.filename
280 for file_info in release.files]
281 self.assertEqual(len(release_filenames), 2)
282 self.assertTrue(file_name in release_filenames)
283 self.assertTrue(alt_file_name in release_filenames)
284
277 def test_handleReleaseUnableToParseVersion(self):285 def test_handleReleaseUnableToParseVersion(self):
278 # Test that handleRelease() handles the case where a version can't be286 # Test that handleRelease() handles the case where a version can't be
279 # parsed from the url given.287 # parsed from the url given.