Merge lp:~mars/tarmac/add-branch-config-get into lp:~launchpad/tarmac/lp-tarmac

Proposed by Māris Fogels on 2010-12-16
Status: Merged
Approved by: Māris Fogels on 2010-12-16
Approved revision: 384
Merged at revision: 388
Proposed branch: lp:~mars/tarmac/add-branch-config-get
Merge into: lp:~launchpad/tarmac/lp-tarmac
Diff against target: 125 lines (+65/-5)
4 files modified
tarmac/config.py (+7/-0)
tarmac/plugins/allowedcontributors.py (+23/-3)
tarmac/plugins/tests/test_allowedcontributors.py (+15/-2)
tarmac/tests/test_config.py (+20/-0)
To merge this branch: bzr merge lp:~mars/tarmac/add-branch-config-get
Reviewer Review Type Date Requested Status
Māris Fogels (community) Approve on 2010-12-16
Review via email: mp+43856@code.launchpad.net

Commit message

Adds a get() method to the BranchConfig object and fixes allowedcontributors.py plugin to handle private teams.

Description of the change

Hello,

This branch adds a get() method to the BranchConfig object. This makes tests for keys in a BranchConfig easier to write. You no longer have to mess around with hasattr() and conditional statements for optional BranchConfig keys (for example, to conditionally activate a plugin based on a branch config key's presence).

The implementation is currently a simple wrapper for getattr(). Later we may want to proxy a proper dict object, but that raises the problem of clashes between the dict method names and the config object key names. It is not something we need yet.

Thanks,

Maris

To post a comment you must log in.
Māris Fogels (mars) wrote :

Self-approved for the Launchpad Tarmac trial and landing on the lp-tarmac fork.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'tarmac/config.py'
2--- tarmac/config.py 2010-10-12 17:52:27 +0000
3+++ tarmac/config.py 2010-12-16 02:29:48 +0000
4@@ -125,3 +125,10 @@
5 if config.has_section(branch_name):
6 for key, val in config.items(branch_name):
7 setattr(self, key, val)
8+
9+ def get(self, attr, default=None):
10+ '''A convenient method for getting a config key that may be missing.
11+
12+ Defaults to None if the key is not set.
13+ '''
14+ return getattr(self, attr, default)
15
16=== modified file 'tarmac/plugins/allowedcontributors.py'
17--- tarmac/plugins/allowedcontributors.py 2010-09-20 18:25:54 +0000
18+++ tarmac/plugins/allowedcontributors.py 2010-12-16 02:29:48 +0000
19@@ -23,6 +23,10 @@
20 """Error for when a contributor does not meet validation requirements."""
21
22
23+class InvalidPersonOrTeam(TarmacMergeError):
24+ """Error for when a required team could not be found."""
25+
26+
27 class AllowedContributors(TarmacPlugin):
28 """Tarmac plug-in for checking whether a contributor is allowed to.
29
30@@ -52,9 +56,25 @@
31 in_team = False
32 for team in self.allowed_contributors:
33 launchpad = command.launchpad
34- lp_members = launchpad.people[team].getMembersByStatus(
35- status=[u'Approved', u'Administrator'])
36- members = [x.name for x in lp_members]
37+ try:
38+ lp_members = launchpad.people[team].getMembersByStatus(
39+ status=u'Approved')
40+ lp_members.extend(
41+ launchpad.people[team].getMembersByStatus(
42+ status=u'Administrator'))
43+ members = [x.name for x in lp_members]
44+ except KeyError:
45+ message = (u'Could not find person or team "%s" on '
46+ u'Launchpad.' % team)
47+ comment = (u'Merging into %(target) requires that '
48+ u'contributing authors be a member of an '
49+ u'acceptable team, or a specified person. '
50+ u'However, the person or team "%(team)s" '
51+ u'was not found on Launchpad.' % {
52+ 'target': proposal.target_branch.display_name,
53+ 'team': team})
54+ raise InvalidPersonOrTeam(message, comment)
55+
56 if author in members:
57 in_team = True
58 break
59
60=== modified file 'tarmac/plugins/tests/test_allowedcontributors.py'
61--- tarmac/plugins/tests/test_allowedcontributors.py 2010-09-20 18:25:54 +0000
62+++ tarmac/plugins/tests/test_allowedcontributors.py 2010-12-16 02:29:48 +0000
63@@ -16,7 +16,7 @@
64 """Tests for the Allowed Contributors plug-in."""
65
66 from tarmac.plugins.allowedcontributors import (
67- InvalidContributor, AllowedContributors)
68+ InvalidContributor, InvalidPersonOrTeam, AllowedContributors)
69 from tarmac.tests import TarmacTestCase
70 from tarmac.tests.mock import Thing
71
72@@ -47,7 +47,7 @@
73 Thing(name=u'person3', status=u'Approved'),
74 Thing(name=u'person8', status=u'Invited'),
75 Thing(name=u'personX', status=u'Expired')]
76- return [x for x in members if x.status in status]
77+ return [x for x in members if x.status == status]
78
79 def noMembersByStatus(self, status=None):
80 """Fake method to not return any members for people."""
81@@ -74,3 +74,16 @@
82 self.plugin.run,
83 command=command, target=target, source=source,
84 proposal=self.proposal)
85+
86+ def test_run_private_team(self):
87+ """Test that the plug-in runs correctly."""
88+ config = Thing(allowed_contributors=u'private_team')
89+ source = Thing(authors=[u'person1', u'person2', u'person3'])
90+ target = Thing(config=config)
91+ launchpad = Thing(people=self.people)
92+ command = Thing(launchpad=launchpad)
93+ self.assertRaises(InvalidPersonOrTeam,
94+ self.plugin.run,
95+ command=command, target=target, source=source,
96+ proposal=self.proposal)
97+
98
99=== modified file 'tarmac/tests/test_config.py'
100--- tarmac/tests/test_config.py 2010-10-12 17:52:27 +0000
101+++ tarmac/tests/test_config.py 2010-12-16 02:29:48 +0000
102@@ -61,3 +61,23 @@
103 self.assertRaises(NoOptionError,
104 self.config.get, 'test', 'test_option')
105 self.config.remove_section('test')
106+
107+
108+class BranchConfigTestCase(TarmacTestCase):
109+ '''Tests for the tarmac.config.BranchConfig object.'''
110+
111+ def test_get_value(self):
112+ expected_value = 'test value'
113+ self.config.add_section('lp:test_get_value')
114+ self.config.set('lp:test_get_value', 'test_key', expected_value)
115+
116+ config = BranchConfig('lp:test_get_value', self.config)
117+
118+ self.assertTrue(hasattr(config, 'test_key'))
119+ self.assertEqual(config.test_key, expected_value)
120+ self.assertEqual(config.get('test_key'), expected_value)
121+
122+ def test_get_unset_value_returns_none(self):
123+ config = BranchConfig('lp:test_no_keys', self.config)
124+ self.assertFalse(hasattr(config, 'missing_key'))
125+ self.assertIs(None, config.get('missing_key'))

Subscribers

People subscribed via source and target branches

to all changes: