Bazaar PQM Plugin

Code review comment for lp:~ursinha/bzr-pqm/add-noqa-incr-lpland

add-noqa-incr-lpland
Merge into devel

Revision history for this message

John A Meinel (jameinel) wrote on 2010-08-06:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Ursula Junque wrote:
> Ursula Junque has proposed merging lp:~ursinha/bzr-pqm/add-noqa-incr-lpland into lp:bzr-pqm.
>
> Requested reviews:
> Bzr-pqm-devel (bzr-pqm-devel)
> Related bugs:
> #602137 add bzr lp-land support to no-qa and incr QA tags
> https://bugs.launchpad.net/bugs/602137
>
>
> = Summary =
>
> This branch is a fix for bug 602137, adding to bzr lp-land three options: --testfix, --no-qa and --incr. --no-qa option should add the [no-qa] tag to the commit message, --incremental option should add the [incr] tag, and --testfix, [testfix]. This enforces the QA policy of now allowing fixes to be committed without bugs unless explicitely pointed.
>
> == Proposed fix ==
>
> The implementation is pretty simple. A method checks if the --no-qa, --incremental or --testfix options are given to bzr lp-land, and combined with the bugs found in the mp decides if the merge can continue based on the QA policy:
>
> * If the mp has no linked bugs and no no-qa tag, ec2 land should fail because without bugs and no-qa tag that commit would be an orphaned one.
> * If the mp has bugs and the no-qa commit tag, that's ok. The bug #s and the no-qa tag will be added to the commit message. This will be properly parsed and handled by qa-tagger script after the branch landed.
> * If the mp has no linked bugs but has the no-qa commit tag, that's ok. The commit will be qa-untestable, therefore not orphaned.
> * If the mp has no linked bugs but has the incr commit tag, ec2 land should fail, because bugs are needed when the fix is incremental.
> * A commit cannot be no-qa and incr at the same time.

As far as reviewing goes, I'm going to trust that you know the Launchpad
requirements more than I do. As such, I'm not really auditing this part.
I'll just look at the code itself.

Personally, I really don't like this notation:
+__metaclass__ = type

Certainly in bzrlib itself we always do:

class Foo(object):

instead.

I'm certainly inclined to be less strict in bzr-pqm, esp. as I think the
Launchpad standard way of doing it is with __metaclass__. (Also, I see
that test_lpland was using __metaclass__ as well.)

I'm going to just land this, but mostly I'm trusting that you've thought
the logic through. Certainly everything I've seen seems well thought out.

John
=:->
merge: approve

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkxcPVQACgkQJdeBCYSNAAMDNQCcC6688GqhnisqHAjqVti2q3T/
L6kAnivByx3OiMqv8oDA4sV4ZTgosN0Z
=zgkD
-----END PGP SIGNATURE-----

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Ursula Junque wrote:
> Ursula Junque has proposed merging lp:~ursinha/bzr-pqm/add-noqa-incr-lpland into lp:bzr-pqm.
> 
> Requested reviews:
>   Bzr-pqm-devel (bzr-pqm-devel)
> Related bugs:
>   #602137 add bzr lp-land support to no-qa and incr QA tags
>   https://bugs.launchpad.net/bugs/602137
> 
> 
> = Summary =
> 
> This branch is a fix for bug 602137, adding to bzr lp-land three options: --testfix, --no-qa and --incr. --no-qa option should add the [no-qa] tag to the commit message, --incremental option should add the [incr] tag, and --testfix, [testfix]. This enforces the QA policy of now allowing fixes to be committed without bugs unless explicitely pointed.
> 
> == Proposed fix ==
> 
> The implementation is pretty simple. A method checks if the --no-qa, --incremental or --testfix options are given to bzr lp-land, and combined with the bugs found in the mp decides if the merge can continue based on the QA policy:
> 
>  * If the mp has no linked bugs and no no-qa tag, ec2 land should fail because without bugs and no-qa tag that commit would be an orphaned one.
>  * If the mp has bugs and the no-qa commit tag, that's ok. The bug #s and the no-qa tag will be added to the commit message. This will be properly parsed and handled by qa-tagger script after the branch landed.
>  * If the mp has no linked bugs but has the no-qa commit tag, that's ok. The commit will be qa-untestable, therefore not orphaned.
>  * If the mp has no linked bugs but has the incr commit tag, ec2 land should fail, because bugs are needed when the fix is incremental.
>  * A commit cannot be no-qa and incr at the same time.

As far as reviewing goes, I'm going to trust that you know the Launchpad
requirements more than I do. As such, I'm not really auditing this part.
I'll just look at the code itself.

Personally, I really don't like this notation:
+__metaclass__ = type

Certainly in bzrlib itself we always do:

class Foo(object):

instead.

I'm certainly inclined to be less strict in bzr-pqm, esp. as I think the
Launchpad standard way of doing it is with __metaclass__. (Also, I see
that test_lpland was using __metaclass__ as well.)

I'm going to just land this, but mostly I'm trusting that you've thought
the logic through. Certainly everything I've seen seems well thought out.

John
=:->
 merge: approve

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkxcPVQACgkQJdeBCYSNAAMDNQCcC6688GqhnisqHAjqVti2q3T/
L6kAnivByx3OiMqv8oDA4sV4ZTgosN0Z
=zgkD
-----END PGP SIGNATURE-----

review: Approve

« Back to merge proposal