Launchpad itself

Merge lp:~thumper/launchpad/recipe-build-email-fix into lp:launchpad

  1. recipe-build-email-fix
  2. Merge into devel
Proposed by Tim Penhey
Status: Merged
Approved by: Robert Collins
Approved revision: no longer in the source branch.
Merged at revision: 11316
Proposed branch: lp:~thumper/launchpad/recipe-build-email-fix
Merge into: lp:launchpad
Diff against target: 178 lines (+77/-29)
2 files modified
lib/lp/code/model/recipebuilder.py (+8/-2)
lib/lp/code/model/tests/test_recipebuilder.py (+69/-27)
To merge this branch: bzr merge lp:~thumper/launchpad/recipe-build-email-fix
Reviewer Review Type Date Requested Status
Julian Edwards (community) release-critical Approve
Robert Collins (community) Approve
Review via email: mp+32063@code.launchpad.net

Commit message

Handle the case where the build requester does not have a preferred email.

Description of the change

The extra build parameters is currently causing issues on the build master if the owner of a recipe
is a team without an email address specified. It is also possible to get the code into the same
state with a person who has deactivated their account.

The fix just uses a known email address and name for the situations where the build requester does
not have a preferred email.

tests:
  TestRecipeBuilder

To post a comment you must log in.
Revision history for this message
Robert Collins (lifeless) wrote :

I believe I can hand out rc, so rc=lifeless. If I can't please wait for Julian.

A possible tweak would be to use a symbolic name rather than the literal email address.

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

For Soyuz builds we do this:

        fromaddress = format_address(
            config.builddmaster.default_sender_name,
            config.builddmaster.default_sender_address)

which would address mine (and lifeless's) concern about the hard-coded literal.

r/rc=me with that change.

Also, did you look at lib/lp/soyuz/model/binarypackagebuild.py / notify() ? It has a bunch of rules to prevent spamming the innocent, but I think you'll be ok here if you're just notifying the build requester.

Cheers.

review: Approve (release-critical)
Revision history for this message
Julian Edwards (julian-edwards) wrote :

Ok I gather you already landed this from your comment on IRC. It's fine to
file a bug and fix it next cycle.

Cheers.

Revision history for this message
Robert Collins (lifeless) wrote :

It was landed with the literal replaced with a config lookup, so
shouldn't need anything more ?

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

On Monday 09 August 2010 12:11:03 Robert Collins wrote:
> It was landed with the literal replaced with a config lookup, so
> shouldn't need anything more ?

It's not the same config as the Soyuz code uses and we should be consistent.
There's two configs specifically for the builddmaster emails, one for the
sender's email and one for the sender's name. Tim is only using the config
for the email.

Revision history for this message
Robert Collins (lifeless) wrote :

Ok, that makes sense.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/code/model/recipebuilder.py'
--- lib/lp/code/model/recipebuilder.py 2010-07-13 10:20:47 +0000
+++ lib/lp/code/model/recipebuilder.py 2010-08-09 04:58:43 +0000
@@ -63,8 +63,14 @@
63 suite += "-%s" % (self.build.pocket.name.lower())63 suite += "-%s" % (self.build.pocket.name.lower())
64 args['suite'] = suite64 args['suite'] = suite
65 args['arch_tag'] = distroarchseries.architecturetag65 args['arch_tag'] = distroarchseries.architecturetag
66 args["author_name"] = self.build.requester.displayname66 requester = self.build.requester
67 args["author_email"] = self.build.requester.preferredemail.email67 if requester.preferredemail is None:
68 # Use a constant, known, name and email.
69 args["author_name"] = 'Launchpad Package Builder'
70 args["author_email"] = config.canonical.noreply_from_address
71 else:
72 args["author_name"] = requester.displayname
73 args["author_email"] = requester.preferredemail.email
68 args["recipe_text"] = str(self.build.recipe.builder_recipe)74 args["recipe_text"] = str(self.build.recipe.builder_recipe)
69 args['archive_purpose'] = self.build.archive.purpose.name75 args['archive_purpose'] = self.build.archive.purpose.name
70 args["ogrecomponent"] = get_primary_current_component(76 args["ogrecomponent"] = get_primary_current_component(
7177
=== modified file 'lib/lp/code/model/tests/test_recipebuilder.py'
--- lib/lp/code/model/tests/test_recipebuilder.py 2010-07-13 10:20:47 +0000
+++ lib/lp/code/model/tests/test_recipebuilder.py 2010-08-09 04:58:43 +0000
@@ -3,17 +3,17 @@
33
4"""Test RecipeBuildBehavior."""4"""Test RecipeBuildBehavior."""
55
6from __future__ import with_statement
7
6# pylint: disable-msg=F04018# pylint: disable-msg=F0401
79
8__metaclass__ = type10__metaclass__ = type
911
10import re
11import transaction12import transaction
12import unittest13import unittest
1314
14from zope.security.proxy import removeSecurityProxy15from zope.security.proxy import removeSecurityProxy
1516
16from canonical.config import config
17from canonical.testing import LaunchpadFunctionalLayer17from canonical.testing import LaunchpadFunctionalLayer
18from canonical.launchpad.scripts.logger import BufferLogger18from canonical.launchpad.scripts.logger import BufferLogger
1919
@@ -34,7 +34,7 @@
34 MockBuilder, OkSlave)34 MockBuilder, OkSlave)
35from lp.soyuz.tests.test_publishing import (35from lp.soyuz.tests.test_publishing import (
36 SoyuzTestPublisher,)36 SoyuzTestPublisher,)
37from lp.testing import TestCaseWithFactory37from lp.testing import person_logged_in, TestCaseWithFactory
3838
3939
40class TestRecipeBuilder(TestCaseWithFactory):40class TestRecipeBuilder(TestCaseWithFactory):
@@ -52,7 +52,7 @@
52 job = IBuildFarmJobBehavior(job)52 job = IBuildFarmJobBehavior(job)
53 self.assertProvides(job, IBuildFarmJobBehavior)53 self.assertProvides(job, IBuildFarmJobBehavior)
5454
55 def makeJob(self):55 def makeJob(self, recipe_registrant=None, recipe_owner=None):
56 """Create a sample `ISourcePackageRecipeBuildJob`."""56 """Create a sample `ISourcePackageRecipeBuildJob`."""
57 spn = self.factory.makeSourcePackageName("apackage")57 spn = self.factory.makeSourcePackageName("apackage")
58 distro = self.factory.makeDistribution(name="distro")58 distro = self.factory.makeDistribution(name="distro")
@@ -62,15 +62,20 @@
62 distroseries.newArch(62 distroseries.newArch(
63 'i386', processorfamily, True, self.factory.makePerson())63 'i386', processorfamily, True, self.factory.makePerson())
64 sourcepackage = self.factory.makeSourcePackage(spn, distroseries)64 sourcepackage = self.factory.makeSourcePackage(spn, distroseries)
65 requester = self.factory.makePerson(email="requester@ubuntu.com",65 if recipe_registrant is None:
66 name="joe", displayname="Joe User")66 recipe_registrant = self.factory.makePerson(
67 somebranch = self.factory.makeBranch(owner=requester, name="pkg",67 email="requester@ubuntu.com",
68 name="joe", displayname="Joe User")
69 if recipe_owner is None:
70 recipe_owner = recipe_registrant
71 somebranch = self.factory.makeBranch(
72 owner=recipe_owner, name="pkg",
68 product=self.factory.makeProduct("someapp"))73 product=self.factory.makeProduct("someapp"))
69 recipe = self.factory.makeSourcePackageRecipe(requester, requester,74 recipe = self.factory.makeSourcePackageRecipe(
70 distroseries, u"recept", u"Recipe description",75 recipe_registrant, recipe_owner, distroseries, u"recept",
71 branches=[somebranch])76 u"Recipe description", branches=[somebranch])
72 spb = self.factory.makeSourcePackageRecipeBuild(77 spb = self.factory.makeSourcePackageRecipeBuild(
73 sourcepackage=sourcepackage, recipe=recipe, requester=requester,78 sourcepackage=sourcepackage, recipe=recipe, requester=recipe_owner,
74 distroseries=distroseries)79 distroseries=distroseries)
75 job = spb.makeJob()80 job = spb.makeJob()
76 job_id = removeSecurityProxy(job.job).id81 job_id = removeSecurityProxy(job.job).id
@@ -124,15 +129,15 @@
124 AssertionError, job.verifyBuildRequest, BufferLogger())129 AssertionError, job.verifyBuildRequest, BufferLogger())
125 self.assertIn('invalid pocket due to the series status of', str(e))130 self.assertIn('invalid pocket due to the series status of', str(e))
126131
127 def test__extraBuildArgs(self):132 def _setBuilderConfig(self):
133 """Setup a temporary builder config."""
134 self.pushConfig(
135 "builddmaster",
136 bzr_builder_sources_list="deb http://foo %(series)s main")
137
138 def test_extraBuildArgs(self):
128 # _extraBuildArgs will return a sane set of additional arguments139 # _extraBuildArgs will return a sane set of additional arguments
129 bzr_builder_config = """140 self._setBuilderConfig()
130 [builddmaster]
131 bzr_builder_sources_list = deb http://foo %(series)s main
132 """
133 config.push("bzr_builder_config", bzr_builder_config)
134 self.addCleanup(config.pop, "bzr_builder_config")
135
136 job = self.makeJob()141 job = self.makeJob()
137 distroarchseries = job.build.distroseries.architectures[0]142 distroarchseries = job.build.distroseries.architectures[0]
138 expected_archives = get_sources_list_for_building(143 expected_archives = get_sources_list_for_building(
@@ -152,18 +157,55 @@
152 'distroseries_name': job.build.distroseries.name,157 'distroseries_name': job.build.distroseries.name,
153 }, job._extraBuildArgs(distroarchseries))158 }, job._extraBuildArgs(distroarchseries))
154159
160 def test_extraBuildArgs_team_owner_no_email(self):
161 # If the owner of the recipe is a team without a preferred email, the
162 # registrant is used.
163 self._setBuilderConfig()
164 recipe_registrant = self.factory.makePerson(
165 name='eric', displayname='Eric the Viking', email='eric@vikings.r.us')
166 recipe_owner = self.factory.makeTeam(
167 name='vikings', members=[recipe_registrant])
168
169 job = self.makeJob(recipe_registrant, recipe_owner)
170 distroarchseries = job.build.distroseries.architectures[0]
171 extra_args = job._extraBuildArgs(distroarchseries)
172 self.assertEqual("Launchpad Package Builder", extra_args['author_name'])
173 self.assertEqual("noreply@launchpad.net", extra_args['author_email'])
174
175 def test_extraBuildArgs_team_owner_with_email(self):
176 # If the owner of the recipe is a team that has an email set, we use
177 # that.
178 self._setBuilderConfig()
179 recipe_registrant = self.factory.makePerson()
180 recipe_owner = self.factory.makeTeam(
181 name='vikings', email='everyone@vikings.r.us',
182 members=[recipe_registrant])
183
184 job = self.makeJob(recipe_registrant, recipe_owner)
185 distroarchseries = job.build.distroseries.architectures[0]
186 extra_args = job._extraBuildArgs(distroarchseries)
187 self.assertEqual("Vikings", extra_args['author_name'])
188 self.assertEqual("everyone@vikings.r.us", extra_args['author_email'])
189
190 def test_extraBuildArgs_owner_deactivated(self):
191 # If the owner is deactivated, they have no preferred email.
192 self._setBuilderConfig()
193 owner = self.factory.makePerson()
194 with person_logged_in(owner):
195 owner.deactivateAccount('deactivating')
196 job = self.makeJob(owner)
197 distroarchseries = job.build.distroseries.architectures[0]
198 extra_args = job._extraBuildArgs(distroarchseries)
199 self.assertEqual("Launchpad Package Builder", extra_args['author_name'])
200 self.assertEqual("noreply@launchpad.net", extra_args['author_email'])
201
155 def test_extraBuildArgs_withBadConfigForBzrBuilderPPA(self):202 def test_extraBuildArgs_withBadConfigForBzrBuilderPPA(self):
156 # Ensure _extraBuildArgs doesn't blow up with a badly formatted203 # Ensure _extraBuildArgs doesn't blow up with a badly formatted
157 # bzr_builder_sources_list in the config.204 # bzr_builder_sources_list in the config.
158 bzr_builder_config = """205 self.pushConfig(
159 [builddmaster]206 "builddmaster",
160 bzr_builder_sources_list = deb http://foo %(series) main207 bzr_builder_sources_list="deb http://foo %(series) main")
161 """
162 # (note the missing 's' in %(series)208 # (note the missing 's' in %(series)
163
164 config.push("bzr_builder_config", bzr_builder_config)
165 self.addCleanup(config.pop, "bzr_builder_config")
166
167 job = self.makeJob()209 job = self.makeJob()
168 distroarchseries = job.build.distroseries.architectures[0]210 distroarchseries = job.build.distroseries.architectures[0]
169 expected_archives = get_sources_list_for_building(211 expected_archives = get_sources_list_for_building(