Launchpad itself

Merge lp://staging/~rockstar/launchpad/merge-queues-api into lp://staging/launchpad/db-devel

merge-queues-api
Merge into db-devel

Proposed by Paul Hummer on 2010-10-20

Status:

Merged

Merged at revision:

9912

Proposed branch:

lp://staging/~rockstar/launchpad/merge-queues-api

Merge into:

lp://staging/launchpad/db-devel

Prerequisite:

lp://staging/~rockstar/launchpad/merge-queues-model

Diff against target:

535 lines (+248/-67)

12 files modified

lib/lp/code/browser/configure.zcml (+6/-0)
lib/lp/code/configure.zcml (+9/-4)
lib/lp/code/interfaces/branch.py (+32/-16)
lib/lp/code/interfaces/branchmergequeue.py (+60/-39)
lib/lp/code/interfaces/webservice.py (+2/-0)
lib/lp/code/model/tests/test_branch.py (+43/-0)
lib/lp/code/model/tests/test_branchmergequeue.py (+65/-1)
lib/lp/code/stories/webservice/xx-branch.txt (+2/-0)
lib/lp/registry/browser/person.py (+5/-0)
lib/lp/registry/interfaces/person.py (+3/-0)
lib/lp/registry/model/person.py (+7/-0)
lib/lp/testing/factory.py (+14/-7)

To merge this branch:

bzr merge lp://staging/~rockstar/launchpad/merge-queues-api

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Jelmer Vernooij (community)		2010-10-20	Approve on 2010-10-20
Review via email: mp+38946@code.staging.launchpad.net

Description of the change

This branch exposes the new branch merge queue models through the API. Since there's no UI yet, the URL resolving code also needed to be added. While doing this, I also added some more stuff to the factory method for making merge queues.

There's a lot of lint, but most of it isn't something I introduced. I could fix the lint, but it would make the diff hard to read. If you'd like me to, I'll fix the lint, but I think you'd rather me do that after the review. :)

Revision history for this message

Jelmer Vernooij (jelmer) wrote on 2010-10-20:

Hi Paul,

Just a few minor (stylistic) comments. I'm looking forward to being able
to use the merge queues.

On Wed, 2010-10-20 at 15:37 +0000, Paul Hummer wrote:
> There's a lot of lint, but most of it isn't something I introduced. I
> could fix the lint, but it would make the diff hard to read. If you'd
> like me to, I'll fix the lint, but I think you'd rather me do that
> after the review. :)
Thanks for waiting with those fixes. Some lint cleanup would be nice
though. :-)

> differences between files attachment (review-diff.txt)

> === modified file 'lib/lp/code/interfaces/branch.py'
> --- lib/lp/code/interfaces/branch.py 2010-10-20 15:36:54 +0000
> +++ lib/lp/code/interfaces/branch.py 2010-10-20 15:36:56 +0000
> @@ -990,18 +990,6 @@
> required=False, readonly=True,
> vocabulary=ControlFormat))
>
> - merge_queue = Reference(
> - title=_('Branch Merge Queue'),
> - schema=IBranchMergeQueue, required=False, readonly=True,
> - description=_(
> - "The branch merge queue that manages merges for this branch."))
> -
> - merge_queue_config = TextLine(
> - title=_('Name'), required=True, constraint=branch_name_validator,
> - description=_(
> - "A JSON string of configuration values to send to a branch"
> - "merge robot."))
This indentation seems odd - is there a particular reason for not having
the title= and description= lines start at the same column for
merge_queue_config as for merge_queue ?

> === modified file 'lib/lp/registry/model/person.py'
> --- lib/lp/registry/model/person.py 2010-10-18 21:18:03 +0000
> +++ lib/lp/registry/model/person.py 2010-10-20 15:36:56 +0000
> @@ -2661,6 +2661,13 @@
> SourcePackageRecipe, SourcePackageRecipe.owner == self,
> SourcePackageRecipe.name == name).one()
>
> + def getMergeQueue(self, name):
> + from lp.code.model.branchmergequeue import BranchMergeQueue
> + return Store.of(self).find(
> + BranchMergeQueue,
> + BranchMergeQueue.owner == self,
> + BranchMergeQueue.name == unicode(name)).one()
> +
^^ It would be nice to have a "See IPerson" comment here, if only for
consistency with the rest of the file.

review approve

Cheers,

Jelmer

Hi Paul,

Just a few minor (stylistic) comments. I'm looking forward to being able
to use the merge queues.

On Wed, 2010-10-20 at 15:37 +0000, Paul Hummer wrote:
> There's a lot of lint, but most of it isn't something I introduced.  I
>  could fix the lint, but it would make the diff hard to read.  If you'd
>  like me to, I'll fix the lint, but I think you'd rather me do that
>  after the review.  :)
Thanks for waiting with those fixes. Some lint cleanup would be nice
though. :-)

> differences between files attachment (review-diff.txt)

> === modified file 'lib/lp/code/interfaces/branch.py'
> --- lib/lp/code/interfaces/branch.py	2010-10-20 15:36:54 +0000
> +++ lib/lp/code/interfaces/branch.py	2010-10-20 15:36:56 +0000
> @@ -990,18 +990,6 @@
>              required=False, readonly=True,
>              vocabulary=ControlFormat))
>  
> -    merge_queue = Reference(
> -        title=_('Branch Merge Queue'),
> -        schema=IBranchMergeQueue, required=False, readonly=True,
> -        description=_(
> -            "The branch merge queue that manages merges for this branch."))
> -
> -    merge_queue_config = TextLine(
> -            title=_('Name'), required=True, constraint=branch_name_validator,
> -            description=_(
> -                "A JSON string of configuration values to send to a branch"
> -                "merge robot."))
This indentation seems odd - is there a particular reason for not having
the title= and description= lines start at the same column for
merge_queue_config as for merge_queue ?

> === modified file 'lib/lp/registry/model/person.py'
> --- lib/lp/registry/model/person.py	2010-10-18 21:18:03 +0000
> +++ lib/lp/registry/model/person.py	2010-10-20 15:36:56 +0000
> @@ -2661,6 +2661,13 @@
>              SourcePackageRecipe, SourcePackageRecipe.owner == self,
>              SourcePackageRecipe.name == name).one()
>  
> +    def getMergeQueue(self, name):
> +        from lp.code.model.branchmergequeue import BranchMergeQueue
> +        return Store.of(self).find(
> +            BranchMergeQueue,
> +            BranchMergeQueue.owner == self,
> +            BranchMergeQueue.name == unicode(name)).one()
> +
^^ It would be nice to have a "See IPerson" comment here, if only for
consistency with the rest of the file.

review approve

Cheers,

Jelmer

review: Approve