launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04691
[Merge] lp:~abentley/launchpad/upgrade-not-branch-error into lp:launchpad
Aaron Bentley has proposed merging lp:~abentley/launchpad/upgrade-not-branch-error into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #823485 in Launchpad itself: "NotBranchError raised by upgrade_branches.py script"
https://bugs.launchpad.net/launchpad/+bug/823485
For more details, see:
https://code.launchpad.net/~abentley/launchpad/upgrade-not-branch-error/+merge/72242
= Summary =
Fix bug #823485: NotBranchError raised by upgrade_branches.py script
== Proposed fix ==
Handle NotBranchError as a user error and email to user.
== Pre-implementation notes ==
None
== Implementation details ==
The error notification should go to the user who requested the upgrade, but
until now, the requester was not recorded. Now, BranchUpgradeJob.create
requires a requester, and various tests are updated to match.
== Tests ==
bin/test -t test_branch -t test_branchjob -t test_upgrade_branches -t test_garbo
== Demo and Q/A ==
Create a branch. Request an upgrade. Immediately delete the contents of the
branch (but don't delete the branch from the database.)
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/code/browser/branch.py
lib/lp/code/interfaces/branch.py
lib/lp/code/model/tests/test_branchjob.py
database/schema/security.cfg
lib/lp/testing/__init__.py
lib/lp/scripts/tests/test_garbo.py
lib/lp/code/model/branch.py
lib/lp/code/scripts/tests/test_upgrade_branches.py
lib/lp/code/model/tests/test_branch.py
lib/lp/code/model/branchjob.py
--
https://code.launchpad.net/~abentley/launchpad/upgrade-not-branch-error/+merge/72242
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/upgrade-not-branch-error into lp:launchpad.
=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg 2011-08-17 19:51:38 +0000
+++ database/schema/security.cfg 2011-08-19 18:26:41 +0000
@@ -1903,7 +1903,9 @@
groups=script
public.branch = SELECT, UPDATE
public.branchjob = SELECT, INSERT
+public.emailaddress = SELECT
public.job = SELECT, INSERT, UPDATE
+public.person = SELECT
type=user
[send-branch-mail]
=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py 2011-07-21 02:18:54 +0000
+++ lib/lp/code/browser/branch.py 2011-08-19 18:26:41 +0000
@@ -47,7 +47,6 @@
)
from lazr.uri import URI
import pytz
-import simplejson
from zope.app.form.browser import TextAreaWidget
from zope.component import (
getUtility,
@@ -838,7 +837,7 @@
def mirror_of_ssh(self):
"""True if this a mirror branch with an sftp or bzr+ssh URL."""
if not self.context.url:
- return False # not a mirror branch
+ return False # not a mirror branch
uri = URI(self.context.url)
return uri.scheme in ('sftp', 'bzr+ssh')
@@ -994,7 +993,7 @@
@action('Upgrade', name='upgrade_branch')
def upgrade_branch_action(self, action, data):
- self.context.requestUpgrade()
+ self.context.requestUpgrade(self.user)
class BranchEditView(BranchEditFormView, BranchNameValidationMixin):
=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py 2011-08-17 10:01:10 +0000
+++ lib/lp/code/interfaces/branch.py 2011-08-19 18:26:41 +0000
@@ -1063,7 +1063,7 @@
adapted to an IBranchTarget.
"""
- def requestUpgrade():
+ def requestUpgrade(requester):
"""Create an IBranchUpgradeJob to upgrade this branch."""
def branchChanged(stacked_on_url, last_revision_id, control_format,
=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py 2011-08-17 20:21:01 +0000
+++ lib/lp/code/model/branch.py 2011-08-19 18:26:41 +0000
@@ -1152,10 +1152,10 @@
BranchJob.job_type == BranchJobType.UPGRADE_BRANCH)
return jobs.count() > 0
- def requestUpgrade(self):
+ def requestUpgrade(self, requester):
"""See `IBranch`."""
from lp.code.interfaces.branchjob import IBranchUpgradeJobSource
- return getUtility(IBranchUpgradeJobSource).create(self)
+ return getUtility(IBranchUpgradeJobSource).create(self, requester)
def _checkBranchVisibleByUser(self, user):
"""Is *this* branch visible by the user.
=== modified file 'lib/lp/code/model/branchjob.py'
--- lib/lp/code/model/branchjob.py 2011-07-18 20:46:27 +0000
+++ lib/lp/code/model/branchjob.py 2011-08-19 18:26:41 +0000
@@ -21,7 +21,10 @@
from bzrlib.branch import Branch as BzrBranch
from bzrlib.diff import show_diff_trees
-from bzrlib.errors import NoSuchFile
+from bzrlib.errors import (
+ NoSuchFile,
+ NotBranchError,
+ )
from bzrlib.log import (
log_formatter,
show_log,
@@ -107,6 +110,7 @@
from lp.services.job.interfaces.job import JobStatus
from lp.services.job.model.job import Job
from lp.services.job.runner import BaseRunnableJob
+from lp.services.mail.sendmail import format_address_for_person
from lp.translations.interfaces.translationimportqueue import (
ITranslationImportQueue,
)
@@ -275,6 +279,11 @@
vars.append(('branch_name', self.context.branch.unique_name))
return vars
+ def getErrorRecipients(self):
+ if self.requester is None:
+ return []
+ return [format_address_for_person(self.requester)]
+
class BranchDiffJob(BranchJobDerived):
"""A Job that calculates the a diff related to a Branch."""
@@ -361,14 +370,20 @@
classProvides(IBranchUpgradeJobSource)
class_job_type = BranchJobType.UPGRADE_BRANCH
+ user_error_types = (NotBranchError,)
+
+ def getOperationDescription(self):
+ return 'upgrading a branch'
+
@classmethod
- def create(cls, branch):
+ def create(cls, branch, requester):
"""See `IBranchUpgradeJobSource`."""
if not branch.needs_upgrading:
raise AssertionError('Branch does not need upgrading.')
if branch.upgrade_pending:
raise AssertionError('Branch already has upgrade pending.')
- branch_job = BranchJob(branch, BranchJobType.UPGRADE_BRANCH, {})
+ branch_job = BranchJob(
+ branch, BranchJobType.UPGRADE_BRANCH, {}, requester=requester)
return cls(branch_job)
@staticmethod
=== modified file 'lib/lp/code/model/tests/test_branch.py'
--- lib/lp/code/model/tests/test_branch.py 2011-08-09 14:53:07 +0000
+++ lib/lp/code/model/tests/test_branch.py 2011-08-19 18:26:41 +0000
@@ -617,7 +617,7 @@
owner = removeSecurityProxy(branch).owner
login_person(owner)
self.addCleanup(logout)
- branch.requestUpgrade()
+ branch.requestUpgrade(branch.owner)
self.assertFalse(branch.needs_upgrading)
@@ -682,7 +682,7 @@
owner = removeSecurityProxy(branch).owner
login_person(owner)
self.addCleanup(logout)
- job = removeSecurityProxy(branch.requestUpgrade())
+ job = removeSecurityProxy(branch.requestUpgrade(branch.owner))
jobs = list(getUtility(IBranchUpgradeJobSource).iterReady())
self.assertEqual(
@@ -698,7 +698,7 @@
owner = removeSecurityProxy(branch).owner
login_person(owner)
self.addCleanup(logout)
- self.assertRaises(AssertionError, branch.requestUpgrade)
+ self.assertRaises(AssertionError, branch.requestUpgrade, branch.owner)
def test_requestUpgrade_upgrade_pending(self):
# If there is a pending upgrade already requested, requestUpgrade
@@ -708,9 +708,9 @@
owner = removeSecurityProxy(branch).owner
login_person(owner)
self.addCleanup(logout)
- branch.requestUpgrade()
+ branch.requestUpgrade(branch.owner)
- self.assertRaises(AssertionError, branch.requestUpgrade)
+ self.assertRaises(AssertionError, branch.requestUpgrade, branch.owner)
def test_upgradePending(self):
# If there is a BranchUpgradeJob pending for the branch, return True.
@@ -719,7 +719,7 @@
owner = removeSecurityProxy(branch).owner
login_person(owner)
self.addCleanup(logout)
- branch.requestUpgrade()
+ branch.requestUpgrade(branch.owner)
self.assertTrue(branch.upgrade_pending)
@@ -737,7 +737,7 @@
owner = removeSecurityProxy(branch).owner
login_person(owner)
self.addCleanup(logout)
- branch_job = removeSecurityProxy(branch.requestUpgrade())
+ branch_job = removeSecurityProxy(branch.requestUpgrade(branch.owner))
branch_job.job.start()
branch_job.job.complete()
@@ -1430,7 +1430,6 @@
package.distribution.owner,
package.development_version.setBranch,
pocket, branch, package.distribution.owner)
- series_set = getUtility(IFindOfficialBranchLinks)
self.assertEqual(
{package: ('alter',
_('Branch is officially linked to a source package.'))},
=== modified file 'lib/lp/code/model/tests/test_branchjob.py'
--- lib/lp/code/model/tests/test_branchjob.py 2011-08-12 11:37:08 +0000
+++ lib/lp/code/model/tests/test_branchjob.py 2011-08-19 18:26:41 +0000
@@ -15,7 +15,10 @@
Branch,
BzrBranchFormat7,
)
-from bzrlib.bzrdir import BzrDirMetaFormat1
+from bzrlib.bzrdir import (
+ BzrDir,
+ BzrDirMetaFormat1,
+ )
from bzrlib.repofmt.pack_repo import RepositoryFormatKnitPack6
from bzrlib.revision import NULL_REVISION
from bzrlib.transport import get_transport
@@ -77,6 +80,7 @@
from lp.scripts.helpers import TransactionFreeOperation
from lp.services.job.interfaces.job import JobStatus
from lp.services.job.model.job import Job
+from lp.services.job.runner import JobRunner
from lp.services.osutils import override_environ
from lp.testing import TestCaseWithFactory
from lp.testing.mail_helpers import pop_notifications
@@ -291,20 +295,18 @@
branch = self.factory.makeAnyBranch(
branch_format=BranchFormat.BZR_BRANCH_5,
repository_format=RepositoryFormat.BZR_REPOSITORY_4)
- job = BranchUpgradeJob.create(branch)
+ job = BranchUpgradeJob.create(branch, self.factory.makePerson())
verifyObject(IBranchUpgradeJob, job)
def test_upgrades_branch(self):
"""Ensure that a branch with an outdated format is upgraded."""
self.useBzrBranches(direct_database=True)
- db_branch, tree = self.create_branch_and_tree(format='knit')
- db_branch.branch_format = BranchFormat.BZR_BRANCH_5
- db_branch.repository_format = RepositoryFormat.BZR_KNIT_1
+ db_branch, tree = self.create_knit()
self.assertEqual(
tree.branch.repository._format.get_format_string(),
'Bazaar-NG Knit Repository Format 1')
- job = BranchUpgradeJob.create(db_branch)
+ job = BranchUpgradeJob.create(db_branch, self.factory.makePerson())
self.becomeDbUser(config.upgrade_branches.dbuser)
with TransactionFreeOperation.require():
job.run()
@@ -321,15 +323,21 @@
branch = self.factory.makeAnyBranch(
branch_format=BranchFormat.BZR_BRANCH_7,
repository_format=RepositoryFormat.BZR_CHK_2A)
- self.assertRaises(AssertionError, BranchUpgradeJob.create, branch)
+ self.assertRaises(
+ AssertionError, BranchUpgradeJob.create, branch,
+ self.factory.makePerson())
+
+ def create_knit(self):
+ db_branch, tree = self.create_branch_and_tree(format='knit')
+ db_branch.branch_format = BranchFormat.BZR_BRANCH_5
+ db_branch.repository_format = RepositoryFormat.BZR_KNIT_1
+ return db_branch, tree
def test_existing_bzr_backup(self):
# If the target branch already has a backup.bzr dir, the upgrade copy
# should remove it.
self.useBzrBranches(direct_database=True)
- db_branch, tree = self.create_branch_and_tree(format='knit')
- db_branch.branch_format = BranchFormat.BZR_BRANCH_5
- db_branch.repository_format = RepositoryFormat.BZR_KNIT_1
+ db_branch, tree = self.create_knit()
# Add a fake backup.bzr dir
source_branch_transport = get_transport(db_branch.getInternalBzrUrl())
@@ -337,7 +345,7 @@
source_branch_transport.clone('.bzr').copy_tree_to_transport(
source_branch_transport.clone('backup.bzr'))
- job = BranchUpgradeJob.create(db_branch)
+ job = BranchUpgradeJob.create(db_branch, self.factory.makePerson())
self.becomeDbUser(config.upgrade_branches.dbuser)
job.run()
@@ -355,6 +363,27 @@
branch.branchChanged('', 'new-id', None, None, None)
Store.of(branch).flush()
+ def test_not_branch_error(self):
+ self.useBzrBranches(direct_database=True)
+ db_branch, tree = self.create_branch_and_tree()
+ branch2 = BzrDir.create_branch_convenience('.')
+ tree.branch.set_stacked_on_url(branch2.base)
+ branch2.bzrdir.destroy_branch()
+ # Create BranchUpgradeJob manually, because we're trying to upgrade a
+ # branch that doesn't need upgrading.
+ requester = self.factory.makePerson()
+ branch_job = BranchJob(
+ db_branch, BranchJobType.UPGRADE_BRANCH, {}, requester=requester)
+ job = BranchUpgradeJob(branch_job)
+ self.becomeDbUser(config.upgrade_branches.dbuser)
+ runner = JobRunner([job])
+ with self.noOops():
+ runner.runJobHandleError(job)
+ (mail,) = pop_notifications()
+ self.assertEqual(
+ 'Launchpad error while upgrading a branch', mail['subject'])
+ self.assertIn('Not a branch', mail.get_payload(decode=True))
+
class TestRevisionMailJob(TestCaseWithFactory):
"""Tests for BranchDiffJob."""
@@ -898,7 +927,7 @@
self.layer.switchDbUser('branchscanner')
self.updateDBRevisions(db_branch, tree.branch)
expected = (
- u"-"*60 + '\n'
+ u"-" * 60 + '\n'
"revno: 1" '\n'
"committer: Joe Bloggs <joe@xxxxxxxxxxx>" '\n'
"branch nick: %s" '\n'
@@ -912,7 +941,7 @@
job.getRevisionMessage(first_revision, 1), expected)
expected_message = (
- u"-"*60 + '\n'
+ u"-" * 60 + '\n'
"revno: 2" '\n'
"committer: Joe Bloggs <joe@xxxxxxxxxxx>" '\n'
"branch nick: %s" '\n'
@@ -988,7 +1017,7 @@
super(TestRosettaUploadJob, self).setUp()
self.series = None
- def _makeBranchWithTreeAndFile(self, file_name, file_content = None):
+ def _makeBranchWithTreeAndFile(self, file_name, file_content=None):
return self._makeBranchWithTreeAndFiles(((file_name, file_content), ))
def _makeBranchWithTreeAndFiles(self, files):
@@ -1035,7 +1064,7 @@
try:
file_content = file_pair[1]
if file_content is None:
- raise IndexError # Same as if missing.
+ raise IndexError # Same as if missing.
except IndexError:
file_content = self.factory.getUniqueString()
dname = os.path.dirname(file_name)
@@ -1060,7 +1089,7 @@
self.series.branch = self.branch
self.series.translations_autoimport_mode = mode
- def _runJobWithFile(self, import_mode, file_name, file_content = None):
+ def _runJobWithFile(self, import_mode, file_name, file_content=None):
return self._runJobWithFiles(
import_mode, ((file_name, file_content), ))
@@ -1293,7 +1322,7 @@
# iterReady does not return jobs for branches where last_scanned_id
# and last_mirror_id are different.
self._makeBranchWithTreeAndFiles([])
- self.branch.last_scanned_id = NULL_REVISION # Was not scanned yet.
+ self.branch.last_scanned_id = NULL_REVISION # Was not scanned yet.
self._makeProductSeries(
TranslationsBranchImportMode.IMPORT_TEMPLATES)
# Put the job in ready state.
=== modified file 'lib/lp/code/scripts/tests/test_upgrade_branches.py'
--- lib/lp/code/scripts/tests/test_upgrade_branches.py 2010-10-04 19:50:45 +0000
+++ lib/lp/code/scripts/tests/test_upgrade_branches.py 2011-08-19 18:26:41 +0000
@@ -34,7 +34,7 @@
target_tree.branch.repository._format.get_format_string(),
'Bazaar-NG Knit Repository Format 1')
- BranchUpgradeJob.create(target)
+ BranchUpgradeJob.create(target, self.factory.makePerson())
transaction.commit()
retcode, stdout, stderr = run_script(
@@ -62,7 +62,7 @@
target_tree.branch.repository._format.get_format_string(),
'Bazaar-NG Knit Repository Format 1')
- BranchUpgradeJob.create(target)
+ BranchUpgradeJob.create(target, self.factory.makePerson())
transaction.commit()
retcode, stdout, stderr = run_script(
=== modified file 'lib/lp/scripts/tests/test_garbo.py'
--- lib/lp/scripts/tests/test_garbo.py 2011-08-17 10:43:26 +0000
+++ lib/lp/scripts/tests/test_garbo.py 2011-08-19 18:26:41 +0000
@@ -848,7 +848,8 @@
db_branch.branch_format = BranchFormat.BZR_BRANCH_5
db_branch.repository_format = RepositoryFormat.BZR_KNIT_1
Store.of(db_branch).flush()
- branch_job = BranchUpgradeJob.create(db_branch)
+ branch_job = BranchUpgradeJob.create(
+ db_branch, self.factory.makePerson())
branch_job.job.date_finished = THIRTY_DAYS_AGO
self.assertEqual(
@@ -876,13 +877,14 @@
branch_format=BranchFormat.BZR_BRANCH_5,
repository_format=RepositoryFormat.BZR_KNIT_1)
- branch_job = BranchUpgradeJob.create(db_branch)
+ branch_job = BranchUpgradeJob.create(
+ db_branch, self.factory.makePerson())
branch_job.job.date_finished = THIRTY_DAYS_AGO
db_branch2 = self.factory.makeAnyBranch(
branch_format=BranchFormat.BZR_BRANCH_5,
repository_format=RepositoryFormat.BZR_KNIT_1)
- BranchUpgradeJob.create(db_branch2)
+ BranchUpgradeJob.create(db_branch2, self.factory.makePerson())
self.runDaily()
=== modified file 'lib/lp/testing/__init__.py'
--- lib/lp/testing/__init__.py 2011-08-15 09:21:48 +0000
+++ lib/lp/testing/__init__.py 2011-08-19 18:26:41 +0000
@@ -437,13 +437,22 @@
'Events were generated: %s.' % event_list)
return result
+ @contextmanager
+ def noOops(self):
+ oops = errorlog.globalErrorUtility.getLastOopsReport()
+ try:
+ yield
+ finally:
+ self.assertNoNewOops(oops)
+
def assertNoNewOops(self, old_oops):
"""Assert that no oops has been recorded since old_oops."""
oops = errorlog.globalErrorUtility.getLastOopsReport()
if old_oops is None:
self.assertIs(None, oops)
else:
- self.assertEqual(oops.id, old_oops.id)
+ self.assertTrue(
+ oops.id == old_oops.id, 'Oops recorded: %s' % oops.id)
def assertSqlAttributeEqualsDate(self, sql_object, attribute_name, date):
"""Fail unless the value of the attribute is equal to the date.
