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
1=== modified file 'lib/lp/code/model/recipebuilder.py'
2--- lib/lp/code/model/recipebuilder.py 2010-07-13 10:20:47 +0000
3+++ lib/lp/code/model/recipebuilder.py 2010-08-09 04:58:43 +0000
4@@ -63,8 +63,14 @@
5 suite += "-%s" % (self.build.pocket.name.lower())
6 args['suite'] = suite
7 args['arch_tag'] = distroarchseries.architecturetag
8- args["author_name"] = self.build.requester.displayname
9- args["author_email"] = self.build.requester.preferredemail.email
10+ requester = self.build.requester
11+ if requester.preferredemail is None:
12+ # Use a constant, known, name and email.
13+ args["author_name"] = 'Launchpad Package Builder'
14+ args["author_email"] = config.canonical.noreply_from_address
15+ else:
16+ args["author_name"] = requester.displayname
17+ args["author_email"] = requester.preferredemail.email
18 args["recipe_text"] = str(self.build.recipe.builder_recipe)
19 args['archive_purpose'] = self.build.archive.purpose.name
20 args["ogrecomponent"] = get_primary_current_component(
21
22=== modified file 'lib/lp/code/model/tests/test_recipebuilder.py'
23--- lib/lp/code/model/tests/test_recipebuilder.py 2010-07-13 10:20:47 +0000
24+++ lib/lp/code/model/tests/test_recipebuilder.py 2010-08-09 04:58:43 +0000
25@@ -3,17 +3,17 @@
26
27 """Test RecipeBuildBehavior."""
28
29+from __future__ import with_statement
30+
31 # pylint: disable-msg=F0401
32
33 __metaclass__ = type
34
35-import re
36 import transaction
37 import unittest
38
39 from zope.security.proxy import removeSecurityProxy
40
41-from canonical.config import config
42 from canonical.testing import LaunchpadFunctionalLayer
43 from canonical.launchpad.scripts.logger import BufferLogger
44
45@@ -34,7 +34,7 @@
46 MockBuilder, OkSlave)
47 from lp.soyuz.tests.test_publishing import (
48 SoyuzTestPublisher,)
49-from lp.testing import TestCaseWithFactory
50+from lp.testing import person_logged_in, TestCaseWithFactory
51
52
53 class TestRecipeBuilder(TestCaseWithFactory):
54@@ -52,7 +52,7 @@
55 job = IBuildFarmJobBehavior(job)
56 self.assertProvides(job, IBuildFarmJobBehavior)
57
58- def makeJob(self):
59+ def makeJob(self, recipe_registrant=None, recipe_owner=None):
60 """Create a sample `ISourcePackageRecipeBuildJob`."""
61 spn = self.factory.makeSourcePackageName("apackage")
62 distro = self.factory.makeDistribution(name="distro")
63@@ -62,15 +62,20 @@
64 distroseries.newArch(
65 'i386', processorfamily, True, self.factory.makePerson())
66 sourcepackage = self.factory.makeSourcePackage(spn, distroseries)
67- requester = self.factory.makePerson(email="requester@ubuntu.com",
68- name="joe", displayname="Joe User")
69- somebranch = self.factory.makeBranch(owner=requester, name="pkg",
70+ if recipe_registrant is None:
71+ recipe_registrant = self.factory.makePerson(
72+ email="requester@ubuntu.com",
73+ name="joe", displayname="Joe User")
74+ if recipe_owner is None:
75+ recipe_owner = recipe_registrant
76+ somebranch = self.factory.makeBranch(
77+ owner=recipe_owner, name="pkg",
78 product=self.factory.makeProduct("someapp"))
79- recipe = self.factory.makeSourcePackageRecipe(requester, requester,
80- distroseries, u"recept", u"Recipe description",
81- branches=[somebranch])
82+ recipe = self.factory.makeSourcePackageRecipe(
83+ recipe_registrant, recipe_owner, distroseries, u"recept",
84+ u"Recipe description", branches=[somebranch])
85 spb = self.factory.makeSourcePackageRecipeBuild(
86- sourcepackage=sourcepackage, recipe=recipe, requester=requester,
87+ sourcepackage=sourcepackage, recipe=recipe, requester=recipe_owner,
88 distroseries=distroseries)
89 job = spb.makeJob()
90 job_id = removeSecurityProxy(job.job).id
91@@ -124,15 +129,15 @@
92 AssertionError, job.verifyBuildRequest, BufferLogger())
93 self.assertIn('invalid pocket due to the series status of', str(e))
94
95- def test__extraBuildArgs(self):
96+ def _setBuilderConfig(self):
97+ """Setup a temporary builder config."""
98+ self.pushConfig(
99+ "builddmaster",
100+ bzr_builder_sources_list="deb http://foo %(series)s main")
101+
102+ def test_extraBuildArgs(self):
103 # _extraBuildArgs will return a sane set of additional arguments
104- bzr_builder_config = """
105- [builddmaster]
106- bzr_builder_sources_list = deb http://foo %(series)s main
107- """
108- config.push("bzr_builder_config", bzr_builder_config)
109- self.addCleanup(config.pop, "bzr_builder_config")
110-
111+ self._setBuilderConfig()
112 job = self.makeJob()
113 distroarchseries = job.build.distroseries.architectures[0]
114 expected_archives = get_sources_list_for_building(
115@@ -152,18 +157,55 @@
116 'distroseries_name': job.build.distroseries.name,
117 }, job._extraBuildArgs(distroarchseries))
118
119+ def test_extraBuildArgs_team_owner_no_email(self):
120+ # If the owner of the recipe is a team without a preferred email, the
121+ # registrant is used.
122+ self._setBuilderConfig()
123+ recipe_registrant = self.factory.makePerson(
124+ name='eric', displayname='Eric the Viking', email='eric@vikings.r.us')
125+ recipe_owner = self.factory.makeTeam(
126+ name='vikings', members=[recipe_registrant])
127+
128+ job = self.makeJob(recipe_registrant, recipe_owner)
129+ distroarchseries = job.build.distroseries.architectures[0]
130+ extra_args = job._extraBuildArgs(distroarchseries)
131+ self.assertEqual("Launchpad Package Builder", extra_args['author_name'])
132+ self.assertEqual("noreply@launchpad.net", extra_args['author_email'])
133+
134+ def test_extraBuildArgs_team_owner_with_email(self):
135+ # If the owner of the recipe is a team that has an email set, we use
136+ # that.
137+ self._setBuilderConfig()
138+ recipe_registrant = self.factory.makePerson()
139+ recipe_owner = self.factory.makeTeam(
140+ name='vikings', email='everyone@vikings.r.us',
141+ members=[recipe_registrant])
142+
143+ job = self.makeJob(recipe_registrant, recipe_owner)
144+ distroarchseries = job.build.distroseries.architectures[0]
145+ extra_args = job._extraBuildArgs(distroarchseries)
146+ self.assertEqual("Vikings", extra_args['author_name'])
147+ self.assertEqual("everyone@vikings.r.us", extra_args['author_email'])
148+
149+ def test_extraBuildArgs_owner_deactivated(self):
150+ # If the owner is deactivated, they have no preferred email.
151+ self._setBuilderConfig()
152+ owner = self.factory.makePerson()
153+ with person_logged_in(owner):
154+ owner.deactivateAccount('deactivating')
155+ job = self.makeJob(owner)
156+ distroarchseries = job.build.distroseries.architectures[0]
157+ extra_args = job._extraBuildArgs(distroarchseries)
158+ self.assertEqual("Launchpad Package Builder", extra_args['author_name'])
159+ self.assertEqual("noreply@launchpad.net", extra_args['author_email'])
160+
161 def test_extraBuildArgs_withBadConfigForBzrBuilderPPA(self):
162 # Ensure _extraBuildArgs doesn't blow up with a badly formatted
163 # bzr_builder_sources_list in the config.
164- bzr_builder_config = """
165- [builddmaster]
166- bzr_builder_sources_list = deb http://foo %(series) main
167- """
168+ self.pushConfig(
169+ "builddmaster",
170+ bzr_builder_sources_list="deb http://foo %(series) main")
171 # (note the missing 's' in %(series)
172-
173- config.push("bzr_builder_config", bzr_builder_config)
174- self.addCleanup(config.pop, "bzr_builder_config")
175-
176 job = self.makeJob()
177 distroarchseries = job.build.distroseries.architectures[0]
178 expected_archives = get_sources_list_for_building(