Launchpad itself

Merge lp:~jelmer/launchpad/bug499115 into lp:launchpad/db-devel

  1. bug499115
  2. Merge into db-devel
Proposed by Jelmer Vernooij
Status: Rejected
Rejected by: Jelmer Vernooij
Proposed branch: lp:~jelmer/launchpad/bug499115
Merge into: lp:launchpad/db-devel
Diff against target: 140 lines (+59/-18) (has conflicts)
2 files modified
lib/lp/buildmaster/model/buildbase.py (+30/-18)
lib/lp/buildmaster/tests/test_buildbase.py (+29/-0)
Text conflict in lib/lp/buildmaster/model/buildbase.py
To merge this branch: bzr merge lp:~jelmer/launchpad/bug499115
Reviewer Review Type Date Requested Status
Henning Eggers (community) code Needs Fixing
Review via email: mp+19328@code.launchpad.net

This proposal supersedes a proposal from 2010-02-11.

To post a comment you must log in.
Revision history for this message
Jelmer Vernooij (jelmer) wrote : Posted in a previous version of this proposal

This trivial change prevents the buildd master from falling over if a container directory was created but no log file uploaded. (bug 499115)

Revision history for this message
Henning Eggers (henninge) wrote :

Thank you for adding this simple fix. I still think this corner case should be tested in a unit test. To make testing possible, I'd suggest that you split out the lines under "# Retrieve log file content" into its own method "get_logfile_content" and add tests for it to test/test_bulidbase.py.

review: Needs Fixing (code)
Revision history for this message
Jelmer Vernooij (jelmer) wrote :

Superseded by https://code.edge.launchpad.net/~jelmer/launchpad/bug499115/+merge/20465

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/buildmaster/model/buildbase.py'
2--- lib/lp/buildmaster/model/buildbase.py 2010-02-16 02:27:24 +0000
3+++ lib/lp/buildmaster/model/buildbase.py 2010-03-03 02:43:34 +0000
4@@ -3,6 +3,8 @@
5
6 # pylint: disable-msg=E0211,E0213
7
8+from __future__ import with_statement
9+
10 """Common build base classes."""
11
12 __metaclass__ = type
13@@ -32,6 +34,9 @@
14 from lp.soyuz.model.buildqueue import BuildQueue
15
16
17+UPLOAD_LOG_FILENAME = 'uploader.log'
18+
19+
20 class BuildBase:
21 """A mixin class providing functionality for farm jobs that build a
22 package.
23@@ -96,6 +101,7 @@
24 return None
25 return self._getProxiedFileURL(self.buildlog)
26
27+<<<<<<< TREE
28 @property
29 def upload_log_url(self):
30 """See `IBuildBase`."""
31@@ -103,6 +109,27 @@
32 return None
33 return self._getProxiedFileURL(self.upload_log)
34
35+=======
36+ def getUploadLogContent(self, root, leaf):
37+ """Retrieve the upload log contents.
38+
39+ :param root: Root directory for the uploads
40+ :param leaf: Leaf for this particular upload
41+ :return: Contents of log file or message saying no log file was found.
42+ """
43+ # Retrieve log file content.
44+ possible_locations = (
45+ 'failed', 'failed-to-move', 'rejected', 'accepted')
46+ for location_dir in possible_locations:
47+ log_filepath = os.path.join(root, location_dir, leaf,
48+ UPLOAD_LOG_FILENAME)
49+ if os.path.exists(log_filepath):
50+ with open(log_filepath, 'r') as uploader_log_file:
51+ return uploader_log_file.read()
52+ else:
53+ return 'Could not find upload log file'
54+
55+>>>>>>> MERGE-SOURCE
56 def handleStatus(self, status, librarian, slave_status):
57 """See `IBuildBase`."""
58 logger = logging.getLogger()
59@@ -161,7 +188,7 @@
60 out_file = open(out_file_name, "wb")
61 copy_and_close(slave_file, out_file)
62
63- uploader_logfilename = os.path.join(upload_dir, 'uploader.log')
64+ uploader_logfilename = os.path.join(upload_dir, UPLOAD_LOG_FILENAME)
65 uploader_command = self.getUploaderCommand(
66 upload_leaf, uploader_logfilename)
67 logger.debug("Saving uploader log at '%s'" % uploader_logfilename)
68@@ -239,23 +266,8 @@
69 not self.verifySuccessfulUpload()):
70 logger.debug("Build %s upload failed." % self.id)
71 self.buildstate = BuildStatus.FAILEDTOUPLOAD
72- # Retrieve log file content.
73- possible_locations = (
74- 'failed', 'failed-to-move', 'rejected', 'accepted')
75- for location_dir in possible_locations:
76- upload_final_location = os.path.join(
77- root, location_dir, upload_leaf)
78- if os.path.exists(upload_final_location):
79- log_filepath = os.path.join(
80- upload_final_location, 'uploader.log')
81- uploader_log_file = open(log_filepath)
82- try:
83- uploader_log_content = uploader_log_file.read()
84- finally:
85- uploader_log_file.close()
86- break
87- else:
88- uploader_log_content = 'Could not find upload log file'
89+ uploader_log_content = self.getUploadLogContent(root,
90+ upload_leaf)
91 # Store the upload_log_contents in librarian so it can be
92 # accessed by anyone with permission to see the build.
93 self.storeUploadLog(uploader_log_content)
94
95=== modified file 'lib/lp/buildmaster/tests/test_buildbase.py'
96--- lib/lp/buildmaster/tests/test_buildbase.py 2010-02-03 16:21:57 +0000
97+++ lib/lp/buildmaster/tests/test_buildbase.py 2010-03-03 02:43:34 +0000
98@@ -1,6 +1,8 @@
99 # Copyright 2010 Canonical Ltd. This software is licensed under the
100 # GNU Affero General Public License version 3 (see the file LICENSE).
101
102+from __future__ import with_statement
103+
104 """Tests for `IBuildBase`."""
105
106 __metaclass__ = type
107@@ -45,6 +47,33 @@
108
109 layer = DatabaseFunctionalLayer
110
111+ def test_getUploadLogContent_nolog(self):
112+ """If there is no log file there, a string explaining that is returned.
113+ """
114+ self.useTempDir()
115+ build_base = BuildBase()
116+ self.assertEquals('Could not find upload log file',
117+ build_base.getUploadLogContent(os.getcwd(), "myleaf"))
118+
119+ def test_getUploadLogContent_only_dir(self):
120+ """If there is a directory but no log file, expect the error string,
121+ not an exception."""
122+ self.useTempDir()
123+ os.makedirs("accepted/myleaf")
124+ build_base = BuildBase()
125+ self.assertEquals('Could not find upload log file',
126+ build_base.getUploadLogContent(os.getcwd(), "myleaf"))
127+
128+ def test_getUploadLogContent_readsfile(self):
129+ """If there is a log file, return its contents."""
130+ self.useTempDir()
131+ os.makedirs("accepted/myleaf")
132+ with open('accepted/myleaf/uploader.log', 'w') as f:
133+ f.write('foo')
134+ build_base = BuildBase()
135+ self.assertEquals('foo',
136+ build_base.getUploadLogContent(os.getcwd(), "myleaf"))
137+
138 def test_getUploaderCommand(self):
139 build_base = BuildBase()
140 upload_leaf = self.factory.getUniqueString('upload-leaf')

Subscribers

People subscribed via source and target branches

to status/vote changes: