Merge lp:~adeuring/charm-tools/check-maintainer-branch-owner into lp:~charmers/charm-tools/stable

[Abel Deuring]

This branch adds a check for the maintainer name and the branch owner.

The main idea is that maintainer(s) as specified in metadata.yaml
should also be members of the team that owns the related Launchpad
branch.

I thought a bit if it makes sense to find the intended target
branch of the proofed branch: In theory, it is possible to look for
the parent branch, the "grandparent" branch etc until an LP branch
for a sourcepackage of the "charms" distribution is found. That _could_
be the the final merge target for the given branch, but that's not
always the case -- somebody might create a fork of another branch
with the intention to maintain this fork for a longer time -- and this
is probably the main use case for the new check: To ensure that the
maintainer mentioned in metadata.yaml has write access to the LP
branch of the charm.

Another option would have been to find a push branch on LP and to check
for merge proposals, to eventually find the "right" target. But that is
also not trivial -- and it does not work if the proofed branch has not
yet been pushed to Launchpad.

I other words: Lots of cumbersome "guess work" with limited success
probabilty.

So I took the most simple approach: The user must explicitly
specify the merge target of the given branch.

Currently, the proofer checks if the name of the main charm directory
matches the charm name. This does not make sense for branches, so this
required some changes how the directory name is handled in function run().

I found the distinction between charm_name (a directory path) and
charm_base_name a bit confusing, so I renamed charm_name to charm_path
and charm_basename to charm_name.

It seems that we do not have any infrastructure for unit tests of
functions that access the LP web API, so I wrote also a little mock
for the LP API.

[lp:~adeuring/charm-tools/check-maintainer-branch-owner]

252. By Abel Deuring on 2013-10-07

Make the version of charmtools available in the package itself.

[Abel Deuring (adeuring)]

Another change: I made the package version available in the package itself. The branch changes the parameter passed to the function run() of the proofer. With Charmworld, there is at least one other project that uses the proofer. Having a simple way to discover the version of charmtools makes it easier for charmworld to figure out how to call proof.run().

Unmerged revisions

253. By Abel Deuring on 2013-10-08

reverted all changes except definition of the version number in charmtools/__init__.py

252. By Abel Deuring on 2013-10-07

Make the version of charmtools available in the package itself.

251. By Abel Deuring on 2013-10-01

Check consistency of branch owner and maintainers specified in metadata.yaml

=== modified file 'charmtools/__init__.py'
--- charmtools/__init__.py	2013-09-13 16:38:52 +0000
+++ charmtools/__init__.py	2013-10-07 13:11:20 +0000
@@ -20,6 +20,8 @@
 
 import subprocess
 
+__version__ = '1.1.0'
+
 ext = ''
 if os.name == 'nt':
     ext = '.exe'

=== modified file 'charmtools/proof.py'
--- charmtools/proof.py	2013-09-17 15:31:55 +0000
+++ charmtools/proof.py	2013-10-07 13:11:20 +0000
@@ -17,6 +17,7 @@
 
 import argparse
 import email.utils
+from launchpadlib.launchpad import Launchpad
 import os
 import re
 from stat import ST_MODE
@@ -118,8 +119,8 @@
             return False
 
     def check_relation_hooks(self, relations, subordinate, hooks_path):
-        template_interfaces = ('interface-name')
-        template_relations = ('relation-name')
+        template_interfaces = ('interface-name', )
+        template_relations = ('relation-name', )
 
         for r in relations.items():
             if type(r[1]) != dict:
@@ -239,34 +240,96 @@
                 # value exists.
                 pass
 
+    def check_branch_owner_maintainer(self, branch_owner_name, maintainers):
+        lp = Launchpad.login_with(sys.argv[0], 'production')
+        owners = [
+            person for person in lp.people.find(text=branch_owner_name)
+            if person.name == branch_owner_name]
+        if not owners:
+            self.warn(
+                'Launchpad does not know about the branch owner %s'
+                % branch_owner_name)
+            return
+        [branch_owner] = owners
+        # branch_owner may be a team; check the members in this case.
+        participants = list(branch_owner.participants)
+        if not participants:
+            # lp_person.participants is empty for real persons.
+            participants = [branch_owner]
+        plausible = False
+        for maintainer in maintainers:
+            name, address = email.utils.parseaddr(maintainer)
+            if address != '':
+                lp_person = lp.people.getByEmail(email=address)
+                if lp_person is None:
+                    self.warn(
+                        'Maintainer %s has no Launchpad account' % maintainer)
+                    continue
+                if lp_person in participants:
+                    plausible = True
+                else:
+                    self.warn('Maintainer %s is not the branch owner or '
+                              'a member of the branch owner team (%s).'
+                              % (maintainer, branch_owner_name))
+        if not plausible:
+            self.err(
+                "None of the charm maintainers specified in metadata.yaml "
+                "is a member of the Launchpad branch owner team.")
+
 
 def get_args():
     parser = argparse.ArgumentParser(
         description='Performs static analysis on charms')
-    parser.add_argument('charm_name', nargs='?',
+    parser.add_argument('charm_name', nargs='?', default=os.getcwd(),
                         help='path of charm dir to check. Defaults to PWD')
+    parser.add_argument(
+        '-l', '--lp-branch', nargs=1, dest='lp_branch',
+        help='The URL of a Launchpad branch into which the proofed branch '
+            'will be merged, e.g. '
+            'lp:~your_lp_name/charms/precise/apache2/trunk')
     args = parser.parse_args()
 
-    if args.charm_name:
-        charm_name = args.charm_name
-    else:
-        charm_name = os.getcwd()
-
-    return charm_name
-
-
-def run(charm_name):
+    return args
+
+
+def run(args):
+    charm_arg = args.charm_name
+    # Remove a possibly existing trailing slash from the path name.
+    dirname, basename = os.path.split(charm_arg)
+    if basename == '':
+        charm_arg = dirname
     lint = Linter()
-    if os.path.isdir(charm_name):
-        charm_path = charm_name
+    if os.path.isdir(charm_arg):
+        charm_path = charm_arg
     else:
         charm_home = os.getenv('CHARM_HOME', '.')
-        charm_path = os.path.join(charm_home, charm_name)
+        charm_path = os.path.join(charm_home, charm_arg)
 
     if not os.path.isdir(charm_path):
         lint.crit("%s is not a directory, Aborting" % charm_path)
         return lint.lint, lint.exit_code
 
+    bzr_path = os.path.join(charm_path, '.bzr')
+    branch_owner = None
+    if os.path.isdir(bzr_path):
+        if args.lp_branch is None:
+            lint.warn(
+                "A full check of a charm's branch requires the parameter "
+                "--lp-branch.")
+            charm_name = os.path.basename(charm_path)
+        else:
+            mo = re.search(
+                '^lp:~([^/]+)/charms/[^/]+/([^/]+)', args.lp_branch[0])
+            if mo is None:
+                lint.warn(
+                    "can't check the consistency of the charm's and "
+                    "maintainer's name: Unknown target branch specification.")
+                charm_name = os.path.basename(charm_path)
+            else:
+                branch_owner, charm_name = mo.groups()
+    else:
+        charm_name = os.path.basename(charm_path)
+
     hooks_path = os.path.join(charm_path, 'hooks')
     yaml_path = os.path.join(charm_path, 'metadata.yaml')
     try:
@@ -283,11 +346,14 @@
             if key not in KNOWN_METADATA_KEYS:
                 lint.err("Unknown root metadata field (%s)" % key)
 
-        charm_basename = os.path.basename(charm_path)
-        if charm['name'] != charm_basename:
-            warn_msg = ("metadata name (%s) must match directory name (%s) "
-                        "exactly for local deployment.") % (
-                            charm['name'], charm_basename)
+        if charm['name'] != charm_name:
+            if branch_owner is None:
+                warn_msg = ("metadata name (%s) must match directory name "
+                            "(%s) exactly for local deployment.") % (
+                            charm['name'], charm_name)
+            else:
+                warn_msg = ("metadata name (%s) must match the package name "
+                            "(%s) exactly.") % (charm['name'], charm_name)
             lint.warn(warn_msg)
 
         # summary should be short
@@ -311,6 +377,9 @@
                                     formatted))
                     lint.warn(warn_msg)
 
+            if branch_owner is not None:
+                lint.check_branch_owner_maintainer(branch_owner, maintainers)
+
         if 'categories' not in charm:
             lint.warn('Metadata is missing categories.')
         else:
@@ -436,7 +505,7 @@
         lint.check_hook('stop', hooks_path, required=False, recommended=True)
         lint.check_hook('config-changed', hooks_path, required=False)
     except IOError:
-        lint.err("could not find metadata file for " + charm_name)
+        lint.err("could not find metadata file for " + charm_arg)
         lint.exit_code = -1
 
     rev_path = os.path.join(charm_path, 'revision')
@@ -455,8 +524,8 @@
 
 
 def main():
-    charm_name = get_args()
-    lint, exit_code = run(charm_name)
+    args = get_args()
+    lint, exit_code = run(args)
     if lint:
         print "\n".join(lint)
     sys.exit(exit_code)

=== modified file 'setup.py'
--- setup.py	2013-09-13 16:42:10 +0000
+++ setup.py	2013-10-07 13:11:20 +0000
@@ -10,8 +10,7 @@
 ez_setup.use_setuptools()
 
 from setuptools import setup, find_packages
-
-__version__ = '1.0.0'
+from charmtools import __version__
 
 
 setup(

=== modified file 'tests/proof/test_proof.py'
--- tests/proof/test_proof.py	2013-09-17 15:31:55 +0000
+++ tests/proof/test_proof.py	2013-10-07 13:11:20 +0000
@@ -15,6 +15,7 @@
 #    You should have received a copy of the GNU General Public License
 #    along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+from contextlib import contextmanager
 from os.path import abspath, dirname, join
 from shutil import rmtree
 import sys
@@ -25,9 +26,77 @@
 proof_path = dirname(dirname(dirname(abspath(__file__))))
 proof_path = join(proof_path, 'charmtools')
 sys.path.append(proof_path)
+import proof
 from proof import Linter
 
 
+class BadLPAPICall(Exception):
+    pass
+
+
+def kwargs_only(f):
+    def argcheck(*args, **kwargs):
+        if len(args) > 1:
+            raise BadLPAPICall('The LP API allows only keyword arguments.')
+        return f(*args, **kwargs)
+    return argcheck
+
+
+class FakeLPPerson:
+    def __init__(self, name, displayname, email, participants):
+        self.name = name
+        self.displayname=displayname
+        self.email = email
+        self.participants = participants
+
+
+class FakeLPPeople:
+    def __init__(self, persons):
+        unique_names = set(person.name for person in persons)
+        if len(unique_names) != len(persons):
+            raise ValueError('Person names must be unique')
+        unique_addresses = set(person.email for person in persons)
+        if len(unique_addresses) != len(persons):
+            raise ValueError('Person email adresses must be unique')
+        self.persons = persons
+
+    @kwargs_only
+    def find(self, text):
+        text = text.lower()
+        return [
+            person for person in self.persons
+            if (text in person.name.lower()
+                or text in person.displayname.lower()
+                or text in person.email.lower())
+        ]
+
+    @kwargs_only
+    def getByEmail(self, email):
+        for person in self.persons:
+            if person.email == email:
+                return person
+        return None
+
+
+
+class FakeLaunchpad:
+    """Helper for tests that need to access the Launchpad web API."""
+
+    def __init__(self, persons):
+        self.people = FakeLPPeople(persons)
+
+    def login_with(self, *args):
+        return self
+
+
+@contextmanager
+def make_lp(persons):
+    real_lp = proof.Launchpad
+    proof.Launchpad = FakeLaunchpad(persons)
+    yield
+    proof.Launchpad = real_lp
+
+
 class TestProof(TestCase):
 
     def setUp(self):
@@ -241,6 +310,57 @@
             'the type of the default value is str')
         self.assertEqual(expected, self.linter.lint[0])
 
+    def make_people(self):
+        b_owner = FakeLPPerson(
+            'branch-owner', 'Branch Owner', '[e-mail]', [])
+        j_doe = FakeLPPerson(
+            'johndoe', 'John Doe',' [e-mail]', [])
+        j_doe2 = FakeLPPerson(
+            'johndoe2', 'John Doe (the other one)', '[e-mail]', [])
+        owner_team = FakeLPPerson(
+            'owner-team', 'Owner Team', '[e-mail]',
+            [j_doe, j_doe2])
+        maintainer = FakeLPPerson(
+            'maintainer', 'Main Tainer', '[e-mail]', [])
+        return [b_owner, j_doe, j_doe2, owner_team, maintainer]
+
+    def test_check_branch_owner__owner_not_found(self):
+        with make_lp(self.make_people()):
+            self.linter.check_branch_owner_maintainer(
+                'unknown', ['Bar <[e-mail]>'])
+            self.assertEqual(
+                ['W: Launchpad does not know about the branch owner unknown'],
+                self.linter.lint)
+
+    def test_check_branch_owner__maintainer_unkown(self):
+        with make_lp(self.make_people()):
+            self.linter.check_branch_owner_maintainer(
+                'johndoe', ['Bar <[e-mail]>'])
+            self.assertEqual(
+                [
+                    'W: Maintainer Bar <[e-mail]> has no Launchpad '
+                    'account',
+                    'E: None of the charm maintainers specified in '
+                    'metadata.yaml is a member of the Launchpad branch '
+                    'owner team.'
+                    ],
+                self.linter.lint)
+
+    def test_check_branch_owner__owner_is_team_maint_not_in_team(self):
+        with make_lp(self.make_people()):
+            self.linter.check_branch_owner_maintainer(
+                'owner-team', ['Main Tainer <[e-mail]>'])
+            self.assertEqual(
+                [
+                    'W: Maintainer Main Tainer <[e-mail]> is '
+                    'not the branch owner or a member of the branch owner '
+                    'team (owner-team).',
+                    'E: None of the charm maintainers specified in '
+                    'metadata.yaml is a member of the Launchpad branch '
+                    'owner team.',
+                    ],
+                self.linter.lint)
+
 
 if __name__ == '__main__':
     main()