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
1=== modified file 'lib/lp/registry/scripts/productreleasefinder/finder.py'
2--- lib/lp/registry/scripts/productreleasefinder/finder.py 2009-10-25 16:39:15 +0000
3+++ lib/lp/registry/scripts/productreleasefinder/finder.py 2009-11-02 19:45:23 +0000
4@@ -186,15 +186,6 @@
5 owner=product.owner, datereleased=datetime.now(pytz.UTC))
6 self.log.info("Created new release %s for %s/%s",
7 release_name, product_name, series_name)
8-
9- # If we already have a code tarball, stop here.
10- for fileinfo in release.files:
11- if fileinfo.filetype == UpstreamFileType.CODETARBALL:
12- self.log.debug("%s/%s/%s already has a code tarball",
13- product_name, series_name, release_name)
14- self.ztm.abort()
15- return
16-
17 release.addReleaseFile(
18 filename, file, content_type, uploader=product.owner)
19 self.ztm.commit()
20
21=== modified file 'lib/lp/registry/tests/test_prf_finder.py'
22--- lib/lp/registry/tests/test_prf_finder.py 2009-10-25 16:49:35 +0000
23+++ lib/lp/registry/tests/test_prf_finder.py 2009-11-02 19:45:23 +0000
24@@ -102,6 +102,7 @@
25 def test_handleProduct(self):
26 # test that handleProduct() correctly calls handleRelease()
27 class DummyProductReleaseFinder(ProductReleaseFinder):
28+
29 def __init__(self, ztm, log):
30 ProductReleaseFinder.__init__(self, ztm, log)
31 self.seen_releases = []
32@@ -154,6 +155,16 @@
33
34 layer = LaunchpadZopelessLayer
35
36+ def create_tarball(self, file_name):
37+ """create a release tarball for testing"""
38+ file_path = os.path.join(self.release_root, file_name)
39+ try:
40+ fp = open(file_path, 'w')
41+ fp.write('foo')
42+ finally:
43+ fp.close()
44+ return file_path, file_name
45+
46 def setUp(self):
47 LaunchpadZopelessLayer.switchDbUser(
48 config.productreleasefinder.dbuser)
49@@ -168,14 +179,9 @@
50 ztm = self.layer.txn
51 logging.basicConfig(level=logging.CRITICAL)
52 prf = ProductReleaseFinder(ztm, logging.getLogger())
53- file_name = 'evolution-42.0.orig.tar.gz'
54 alt_file_name = 'evolution-42.0.orig.tar.bz2'
55-
56- # create a release tarball
57- fp = open(os.path.join(
58- self.release_root, file_name), 'w')
59- fp.write('foo')
60- fp.close()
61+ file_path, file_name = self.create_tarball(
62+ 'evolution-42.0.orig.tar.gz')
63
64 self.assertEqual(
65 prf.hasReleaseFile('evolution', 'trunk', '42.0', file_name),
66@@ -184,8 +190,7 @@
67 prf.hasReleaseFile('evolution', 'trunk', '42.0', alt_file_name),
68 False)
69
70- prf.handleRelease(
71- 'evolution', 'trunk', self.release_url + '/' + file_name)
72+ prf.handleRelease('evolution', 'trunk', file_path)
73
74 self.assertEqual(
75 prf.hasReleaseFile('evolution', 'trunk', '42.0', file_name),
76@@ -234,15 +239,8 @@
77
78 logging.basicConfig(level=logging.CRITICAL)
79 prf = ProductReleaseFinder(ztm, logging.getLogger())
80-
81- # create a release tarball
82- fp = open(os.path.join(
83- self.release_root, 'evolution-2.1.6.tar.gz'), 'w')
84- fp.write('foo')
85- fp.close()
86-
87- prf.handleRelease('evolution', 'trunk',
88- self.release_url + '/evolution-2.1.6.tar.gz')
89+ file_path, file_name = self.create_tarball('evolution-2.1.6.tar.gz')
90+ prf.handleRelease('evolution', 'trunk', file_path)
91
92 # verify that we now have files attached to the release:
93 evo = getUtility(IProductSet).getByName('evolution')
94@@ -257,23 +255,33 @@
95 ztm = self.layer.txn
96 logging.basicConfig(level=logging.CRITICAL)
97 prf = ProductReleaseFinder(ztm, logging.getLogger())
98-
99- # create a release tarball
100- fp = open(os.path.join(
101- self.release_root, 'evolution-42.0.tar.gz'), 'w')
102- fp.write('foo')
103- fp.close()
104-
105- prf.handleRelease('evolution', 'trunk',
106- self.release_url + '/evolution-42.0.tar.gz')
107- prf.handleRelease('evolution', 'trunk',
108- self.release_url + '/evolution-42.0.tar.gz')
109-
110+ file_path, file_name = self.create_tarball('evolution-42.0.tar.gz')
111+ prf.handleRelease('evolution', 'trunk', file_path)
112+ prf.handleRelease('evolution', 'trunk', file_path)
113 evo = getUtility(IProductSet).getByName('evolution')
114 trunk = evo.getSeries('trunk')
115 release = trunk.getRelease('42.0')
116 self.assertEqual(release.files.count(), 1)
117
118+ def test_handleRelease_alternate_verstion(self):
119+ """Verify that tar.gz and tar.bz2 versions are both uploaded."""
120+ ztm = self.layer.txn
121+ logging.basicConfig(level=logging.CRITICAL)
122+ prf = ProductReleaseFinder(ztm, logging.getLogger())
123+ file_path, file_name = self.create_tarball('evolution-45.0.tar.gz')
124+ alt_file_path, alt_file_name = self.create_tarball(
125+ 'evolution-45.0.tar.bz2')
126+ prf.handleRelease('evolution', 'trunk', file_path)
127+ prf.handleRelease('evolution', 'trunk', alt_file_path)
128+ evo = getUtility(IProductSet).getByName('evolution')
129+ trunk = evo.getSeries('trunk')
130+ release = trunk.getRelease('45.0')
131+ release_filenames = [file_info.libraryfile.filename
132+ for file_info in release.files]
133+ self.assertEqual(len(release_filenames), 2)
134+ self.assertTrue(file_name in release_filenames)
135+ self.assertTrue(alt_file_name in release_filenames)
136+
137 def test_handleReleaseUnableToParseVersion(self):
138 # Test that handleRelease() handles the case where a version can't be
139 # parsed from the url given.