Merge lp:~jamesbeedy/charm-helpers/add_uid_gid into lp:charm-helpers
- add_uid_gid
- Merge into devel
Status: | Merged |
---|---|
Merged at revision: | 581 |
Proposed branch: | lp:~jamesbeedy/charm-helpers/add_uid_gid |
Merge into: | lp:charm-helpers |
Diff against target: |
190 lines (+103/-8) 2 files modified
charmhelpers/core/host.py (+53/-4) tests/core/test_host.py (+50/-4) |
To merge this branch: | bzr merge lp:~jamesbeedy/charm-helpers/add_uid_gid |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Matt Bruzek | Approve | ||
Review via email:
|
Commit message
Description of the change
Added uid and gid specification functionality for adduser and addgroup.
Modified adduser and addgroup to include optional uid and gid
specification functionality.
Also added 3 new helper functions uid_exists, gid_exists, and group_exists.
Fixes: Bug #1586693
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Matt Bruzek (mbruzek) wrote : | # |
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Matt Bruzek (mbruzek) wrote : | # |
When I tried to 'make test' many of the existing tests fail because the positional arguments are setting the uid argument.
ERROR: tests.core.
-------
_StringException: Traceback (most recent call last):
File "/tmp/add_
return func(*args, **keywargs)
File "/tmp/add_
result = host.adduser(
File "/tmp/add_
user_info = pwd.getpwuid(uid)
TypeError: an integer is required
FAIL: tests.core.
AssertionError: Expected call: check_call(
j', '-g', 'johndoe', 'johndoe'])
Actual call: check_call(
FAIL: tests.core.
AssertionError: Expected call: check_call(
', 'johndoe'])
Actual call: check_call(
FAIL: tests.core.
AssertionError: Expected call: check_call(
j', '-g', 'foo', '-G', 'bar,qux', 'johndoe'])
Actual call: check_call(
The tests need to pass before this change can be accepted.
- 581. By james beedy
-
cleanup
- 582. By James Beedy <jbeedy@virt-test0>
-
syntax
- 583. By James Beedy <jbeedy@virt-test0>
-
Add keyword argument for password in user tests
Modified calls to adduser to specify a keyword
arg for password.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
james beedy (jamesbeedy) wrote : | # |
@mbruzek Thanks for the review. The tests are now passing!
Looking at writing a few tests specific to the uid and gid functionality as well.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Matt Bruzek (mbruzek) wrote : | # |
Hey James, I _just_ created a merge proposal against this branch of code that has fixes and tests passing. Please consider: https:/
Sorry it took so long, I had been interrupted several times with meetings before making this request.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Matt Bruzek (mbruzek) wrote : | # |
> Hey James, I _just_ created a merge proposal against this branch of code that
> has fixes and tests passing. Please consider:
> https:/
>
> Sorry it took so long, I had been interrupted several times with meetings
> before making this request.
- 584. By James Beedy <jbeedy@virt-test0>
-
Merge lp:~mbruzek/charm-helpers/add_uid_gid
Fix text conflict
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Matt Bruzek (mbruzek) wrote : | # |
There was still one lint error, but I fixed that and pushed this change to charm-helpers. Thank you for the contribution!
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Matt Bruzek (mbruzek) wrote : | # |
make tests pass and I am satisfied with the code changes.
Preview Diff
1 | === modified file 'charmhelpers/core/host.py' | |||
2 | --- charmhelpers/core/host.py 2016-05-17 17:41:24 +0000 | |||
3 | +++ charmhelpers/core/host.py 2016-05-31 20:24:20 +0000 | |||
4 | @@ -174,9 +174,8 @@ | |||
5 | 174 | """Return True if the host system uses systemd, False otherwise.""" | 174 | """Return True if the host system uses systemd, False otherwise.""" |
6 | 175 | return os.path.isdir(SYSTEMD_SYSTEM) | 175 | return os.path.isdir(SYSTEMD_SYSTEM) |
7 | 176 | 176 | ||
8 | 177 | |||
9 | 178 | def adduser(username, password=None, shell='/bin/bash', system_user=False, | 177 | def adduser(username, password=None, shell='/bin/bash', system_user=False, |
11 | 179 | primary_group=None, secondary_groups=None): | 178 | primary_group=None, secondary_groups=None, uid=None): |
12 | 180 | """Add a user to the system. | 179 | """Add a user to the system. |
13 | 181 | 180 | ||
14 | 182 | Will log but otherwise succeed if the user already exists. | 181 | Will log but otherwise succeed if the user already exists. |
15 | @@ -187,15 +186,21 @@ | |||
16 | 187 | :param bool system_user: Whether to create a login or system user | 186 | :param bool system_user: Whether to create a login or system user |
17 | 188 | :param str primary_group: Primary group for user; defaults to username | 187 | :param str primary_group: Primary group for user; defaults to username |
18 | 189 | :param list secondary_groups: Optional list of additional groups | 188 | :param list secondary_groups: Optional list of additional groups |
19 | 189 | :param int uid: UID for user being created | ||
20 | 190 | 190 | ||
21 | 191 | :returns: The password database entry struct, as returned by `pwd.getpwnam` | 191 | :returns: The password database entry struct, as returned by `pwd.getpwnam` |
22 | 192 | """ | 192 | """ |
23 | 193 | try: | 193 | try: |
24 | 194 | user_info = pwd.getpwnam(username) | 194 | user_info = pwd.getpwnam(username) |
25 | 195 | log('user {0} already exists!'.format(username)) | 195 | log('user {0} already exists!'.format(username)) |
26 | 196 | if uid: | ||
27 | 197 | user_info = pwd.getpwuid(int(uid)) | ||
28 | 198 | log('user with uid {0} already exists!'.format(uid)) | ||
29 | 196 | except KeyError: | 199 | except KeyError: |
30 | 197 | log('creating user {0}'.format(username)) | 200 | log('creating user {0}'.format(username)) |
31 | 198 | cmd = ['useradd'] | 201 | cmd = ['useradd'] |
32 | 202 | if uid: | ||
33 | 203 | cmd.extend(['--uid', str(uid)]) | ||
34 | 199 | if system_user or password is None: | 204 | if system_user or password is None: |
35 | 200 | cmd.append('--system') | 205 | cmd.append('--system') |
36 | 201 | else: | 206 | else: |
37 | @@ -230,14 +235,58 @@ | |||
38 | 230 | return user_exists | 235 | return user_exists |
39 | 231 | 236 | ||
40 | 232 | 237 | ||
43 | 233 | def add_group(group_name, system_group=False): | 238 | def uid_exists(uid): |
44 | 234 | """Add a group to the system""" | 239 | """Check if a uid exists""" |
45 | 240 | try: | ||
46 | 241 | pwd.getpwuid(uid) | ||
47 | 242 | uid_exists = True | ||
48 | 243 | except KeyError: | ||
49 | 244 | uid_exists = False | ||
50 | 245 | return uid_exists | ||
51 | 246 | |||
52 | 247 | |||
53 | 248 | def group_exists(groupname): | ||
54 | 249 | """Check if a group exists""" | ||
55 | 250 | try: | ||
56 | 251 | grp.getgrnam(groupname) | ||
57 | 252 | group_exists = True | ||
58 | 253 | except KeyError: | ||
59 | 254 | group_exists = False | ||
60 | 255 | return group_exists | ||
61 | 256 | |||
62 | 257 | |||
63 | 258 | def gid_exists(gid): | ||
64 | 259 | """Check if a gid exists""" | ||
65 | 260 | try: | ||
66 | 261 | grp.getgrgid(gid) | ||
67 | 262 | gid_exists = True | ||
68 | 263 | except KeyError: | ||
69 | 264 | gid_exists = False | ||
70 | 265 | return gid_exists | ||
71 | 266 | |||
72 | 267 | |||
73 | 268 | def add_group(group_name, system_group=False, gid=None): | ||
74 | 269 | """Add a group to the system | ||
75 | 270 | |||
76 | 271 | Will log but otherwise succeed if the group already exists. | ||
77 | 272 | |||
78 | 273 | :param str group_name: group to create | ||
79 | 274 | :param bool system_group: Create system group | ||
80 | 275 | :param int gid: GID for user being created | ||
81 | 276 | |||
82 | 277 | :returns: The password database entry struct, as returned by `grp.getgrnam` | ||
83 | 278 | """ | ||
84 | 235 | try: | 279 | try: |
85 | 236 | group_info = grp.getgrnam(group_name) | 280 | group_info = grp.getgrnam(group_name) |
86 | 237 | log('group {0} already exists!'.format(group_name)) | 281 | log('group {0} already exists!'.format(group_name)) |
87 | 282 | if gid: | ||
88 | 283 | group_info = grp.getgrgid(gid) | ||
89 | 284 | log('group with gid {0} already exists!'.format(gid)) | ||
90 | 238 | except KeyError: | 285 | except KeyError: |
91 | 239 | log('creating group {0}'.format(group_name)) | 286 | log('creating group {0}'.format(group_name)) |
92 | 240 | cmd = ['addgroup'] | 287 | cmd = ['addgroup'] |
93 | 288 | if gid: | ||
94 | 289 | cmd.extend(['--gid', str(gid)]) | ||
95 | 241 | if system_group: | 290 | if system_group: |
96 | 242 | cmd.append('--system') | 291 | cmd.append('--system') |
97 | 243 | else: | 292 | else: |
98 | 244 | 293 | ||
99 | === modified file 'tests/core/test_host.py' | |||
100 | --- tests/core/test_host.py 2016-05-17 17:37:13 +0000 | |||
101 | +++ tests/core/test_host.py 2016-05-31 20:24:20 +0000 | |||
102 | @@ -533,7 +533,7 @@ | |||
103 | 533 | 533 | ||
104 | 534 | getpwnam.side_effect = [existing_user_pwnam, new_user_pwnam] | 534 | getpwnam.side_effect = [existing_user_pwnam, new_user_pwnam] |
105 | 535 | 535 | ||
107 | 536 | result = host.adduser(username, password) | 536 | result = host.adduser(username, password=password) |
108 | 537 | 537 | ||
109 | 538 | self.assertEqual(result, new_user_pwnam) | 538 | self.assertEqual(result, new_user_pwnam) |
110 | 539 | check_call.assert_called_with([ | 539 | check_call.assert_called_with([ |
111 | @@ -557,7 +557,7 @@ | |||
112 | 557 | 557 | ||
113 | 558 | getpwnam.return_value = existing_user_pwnam | 558 | getpwnam.return_value = existing_user_pwnam |
114 | 559 | 559 | ||
116 | 560 | result = host.adduser(username, password) | 560 | result = host.adduser(username, password=password) |
117 | 561 | 561 | ||
118 | 562 | self.assertEqual(result, existing_user_pwnam) | 562 | self.assertEqual(result, existing_user_pwnam) |
119 | 563 | self.assertFalse(check_call.called) | 563 | self.assertFalse(check_call.called) |
120 | @@ -578,7 +578,7 @@ | |||
121 | 578 | getpwnam.side_effect = [existing_user_pwnam, new_user_pwnam] | 578 | getpwnam.side_effect = [existing_user_pwnam, new_user_pwnam] |
122 | 579 | getgrnam.side_effect = KeyError('group not found') | 579 | getgrnam.side_effect = KeyError('group not found') |
123 | 580 | 580 | ||
125 | 581 | result = host.adduser(username, password, shell=shell) | 581 | result = host.adduser(username, password=password, shell=shell) |
126 | 582 | 582 | ||
127 | 583 | self.assertEqual(result, new_user_pwnam) | 583 | self.assertEqual(result, new_user_pwnam) |
128 | 584 | check_call.assert_called_with([ | 584 | check_call.assert_called_with([ |
129 | @@ -603,7 +603,7 @@ | |||
130 | 603 | 603 | ||
131 | 604 | getpwnam.side_effect = [existing_user_pwnam, new_user_pwnam] | 604 | getpwnam.side_effect = [existing_user_pwnam, new_user_pwnam] |
132 | 605 | 605 | ||
134 | 606 | result = host.adduser(username, password, | 606 | result = host.adduser(username, password=password, |
135 | 607 | primary_group='foo', secondary_groups=[ | 607 | primary_group='foo', secondary_groups=[ |
136 | 608 | 'bar', 'qux', | 608 | 'bar', 'qux', |
137 | 609 | ]) | 609 | ]) |
138 | @@ -642,6 +642,52 @@ | |||
139 | 642 | getpwnam.assert_called_with(username) | 642 | getpwnam.assert_called_with(username) |
140 | 643 | 643 | ||
141 | 644 | @patch('pwd.getpwnam') | 644 | @patch('pwd.getpwnam') |
142 | 645 | @patch('pwd.getpwuid') | ||
143 | 646 | @patch('grp.getgrnam') | ||
144 | 647 | @patch('subprocess.check_call') | ||
145 | 648 | @patch.object(host, 'log') | ||
146 | 649 | def test_add_user_uid(self, log, check_call, getgrnam, getpwuid, getpwnam): | ||
147 | 650 | user_name = 'james' | ||
148 | 651 | user_id = 1111 | ||
149 | 652 | uid_key_error = KeyError('user not found') | ||
150 | 653 | getpwuid.side_effect = uid_key_error | ||
151 | 654 | host.adduser(user_name, uid=user_id) | ||
152 | 655 | |||
153 | 656 | check_call.assert_called_with([ | ||
154 | 657 | 'useradd', | ||
155 | 658 | '--uid', | ||
156 | 659 | str(user_id), | ||
157 | 660 | '--system', | ||
158 | 661 | '-g', | ||
159 | 662 | user_name, | ||
160 | 663 | user_name | ||
161 | 664 | ]) | ||
162 | 665 | getpwnam.assert_called_with(user_name) | ||
163 | 666 | getpwuid.assert_called_with(user_id) | ||
164 | 667 | |||
165 | 668 | @patch('grp.getgrnam') | ||
166 | 669 | @patch('grp.getgrgid') | ||
167 | 670 | @patch('subprocess.check_call') | ||
168 | 671 | @patch.object(host, 'log') | ||
169 | 672 | def test_add_group_gid(self, log, check_call, getgrgid, getgrnam): | ||
170 | 673 | group_name = 'darkhorse' | ||
171 | 674 | group_id = 1005 | ||
172 | 675 | existing_group_gid = KeyError('group not found') | ||
173 | 676 | new_group_gid = 1006 | ||
174 | 677 | getgrgid.side_effect = [existing_group_gid, new_group_gid] | ||
175 | 678 | |||
176 | 679 | host.add_group(group_name, gid=group_id) | ||
177 | 680 | check_call.assert_called_with([ | ||
178 | 681 | 'addgroup', | ||
179 | 682 | '--gid', | ||
180 | 683 | str(group_id), | ||
181 | 684 | '--group', | ||
182 | 685 | group_name | ||
183 | 686 | ]) | ||
184 | 687 | getgrgid.assert_called_with(group_id) | ||
185 | 688 | getgrnam.assert_called_with(group_name) | ||
186 | 689 | |||
187 | 690 | @patch('pwd.getpwnam') | ||
188 | 645 | def test_user_exists_true(self, getpwnam): | 691 | def test_user_exists_true(self, getpwnam): |
189 | 646 | getpwnam.side_effect = 'pw info' | 692 | getpwnam.side_effect = 'pw info' |
190 | 647 | self.assertTrue(host.user_exists('bob')) | 693 | self.assertTrue(host.user_exists('bob')) |
Thanks for the contribution James! Overall I think this kind of change is a good thing and I am interested to see it land. I see that the new arguments are optional so that will not break backwards compatibility with previous callers of this method.
I found a small problem with the cmd array manipulation. By using .append('--uid 1003') that creates a single array element with a space where we want 2 new elements. If you fix that, I can get behind this change.
Thanks for the work I look forward to the next iteration.