Merge lp:~jamesbeedy/charm-helpers/add_uid_gid into lp:charm-helpers

Proposed by james beedy
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
Reviewer Review Type Date Requested Status
Matt Bruzek Approve
Review via email: mp+295996@code.launchpad.net

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

To post a comment you must log in.
Revision history for this message
Matt Bruzek (mbruzek) wrote :

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.

Revision history for this message
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.test_host.HelpersTest.test_doesnt_add_user_if_it_already_exists
----------------------------------------------------------------------
_StringException: Traceback (most recent call last):
  File "/tmp/add_uid_gid/.venv/local/lib/python2.7/site-packages/mock.py", line 1201, in patched
    return func(*args, **keywargs)
  File "/tmp/add_uid_gid/tests/core/test_host.py", line 560, in test_doesnt_add_user_if_it_already_exists
    result = host.adduser(username, password)
  File "/tmp/add_uid_gid/charmhelpers/core/host.py", line 198, in adduser
    user_info = pwd.getpwuid(uid)
TypeError: an integer is required

FAIL: tests.core.test_host.HelpersTest.test_adds_a_user_if_it_doesnt_exist
AssertionError: Expected call: check_call(['useradd', '--create-home', '--shell', '/bin/bash', '--password', 'eodnho
j', '-g', 'johndoe', 'johndoe'])
Actual call: check_call(['useradd', '--uid eodnhoj', '--system', '-g', 'johndoe', 'johndoe'])

FAIL: tests.core.test_host.HelpersTest.test_adds_a_user_with_different_shell
AssertionError: Expected call: check_call(['useradd', '--create-home', '--shell', '/bin/zsh', '--password', 'eodnhoj
', 'johndoe'])
Actual call: check_call(['useradd', '--uid eodnhoj', '--system', 'johndoe'])

FAIL: tests.core.test_host.HelpersTest.test_adduser_with_groups
AssertionError: Expected call: check_call(['useradd', '--create-home', '--shell', '/bin/bash', '--password', 'eodnho
j', '-g', 'foo', '-G', 'bar,qux', 'johndoe'])
Actual call: check_call(['useradd', '--uid eodnhoj', '--system', '-g', 'foo', '-G', 'bar,qux', 'johndoe'])

The tests need to pass before this change can be accepted.

review: Needs Fixing
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.

Revision history for this message
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.

Revision history for this message
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://code.launchpad.net/~mbruzek/charm-helpers/add_uid_gid/+merge/296130

Sorry it took so long, I had been interrupted several times with meetings before making this request.

Revision history for this message
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://code.launchpad.net/~mbruzek/charm-helpers/add_uid_gid/+merge/296132
>
> 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

Revision history for this message
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!

review: Approve
Revision history for this message
Matt Bruzek (mbruzek) wrote :

make tests pass and I am satisfied with the code changes.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
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 """Return True if the host system uses systemd, False otherwise."""
6 return os.path.isdir(SYSTEMD_SYSTEM)
7
8-
9 def adduser(username, password=None, shell='/bin/bash', system_user=False,
10- primary_group=None, secondary_groups=None):
11+ primary_group=None, secondary_groups=None, uid=None):
12 """Add a user to the system.
13
14 Will log but otherwise succeed if the user already exists.
15@@ -187,15 +186,21 @@
16 :param bool system_user: Whether to create a login or system user
17 :param str primary_group: Primary group for user; defaults to username
18 :param list secondary_groups: Optional list of additional groups
19+ :param int uid: UID for user being created
20
21 :returns: The password database entry struct, as returned by `pwd.getpwnam`
22 """
23 try:
24 user_info = pwd.getpwnam(username)
25 log('user {0} already exists!'.format(username))
26+ if uid:
27+ user_info = pwd.getpwuid(int(uid))
28+ log('user with uid {0} already exists!'.format(uid))
29 except KeyError:
30 log('creating user {0}'.format(username))
31 cmd = ['useradd']
32+ if uid:
33+ cmd.extend(['--uid', str(uid)])
34 if system_user or password is None:
35 cmd.append('--system')
36 else:
37@@ -230,14 +235,58 @@
38 return user_exists
39
40
41-def add_group(group_name, system_group=False):
42- """Add a group to the system"""
43+def uid_exists(uid):
44+ """Check if a uid exists"""
45+ try:
46+ pwd.getpwuid(uid)
47+ uid_exists = True
48+ except KeyError:
49+ uid_exists = False
50+ return uid_exists
51+
52+
53+def group_exists(groupname):
54+ """Check if a group exists"""
55+ try:
56+ grp.getgrnam(groupname)
57+ group_exists = True
58+ except KeyError:
59+ group_exists = False
60+ return group_exists
61+
62+
63+def gid_exists(gid):
64+ """Check if a gid exists"""
65+ try:
66+ grp.getgrgid(gid)
67+ gid_exists = True
68+ except KeyError:
69+ gid_exists = False
70+ return gid_exists
71+
72+
73+def add_group(group_name, system_group=False, gid=None):
74+ """Add a group to the system
75+
76+ Will log but otherwise succeed if the group already exists.
77+
78+ :param str group_name: group to create
79+ :param bool system_group: Create system group
80+ :param int gid: GID for user being created
81+
82+ :returns: The password database entry struct, as returned by `grp.getgrnam`
83+ """
84 try:
85 group_info = grp.getgrnam(group_name)
86 log('group {0} already exists!'.format(group_name))
87+ if gid:
88+ group_info = grp.getgrgid(gid)
89+ log('group with gid {0} already exists!'.format(gid))
90 except KeyError:
91 log('creating group {0}'.format(group_name))
92 cmd = ['addgroup']
93+ if gid:
94+ cmd.extend(['--gid', str(gid)])
95 if system_group:
96 cmd.append('--system')
97 else:
98
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
104 getpwnam.side_effect = [existing_user_pwnam, new_user_pwnam]
105
106- result = host.adduser(username, password)
107+ result = host.adduser(username, password=password)
108
109 self.assertEqual(result, new_user_pwnam)
110 check_call.assert_called_with([
111@@ -557,7 +557,7 @@
112
113 getpwnam.return_value = existing_user_pwnam
114
115- result = host.adduser(username, password)
116+ result = host.adduser(username, password=password)
117
118 self.assertEqual(result, existing_user_pwnam)
119 self.assertFalse(check_call.called)
120@@ -578,7 +578,7 @@
121 getpwnam.side_effect = [existing_user_pwnam, new_user_pwnam]
122 getgrnam.side_effect = KeyError('group not found')
123
124- result = host.adduser(username, password, shell=shell)
125+ result = host.adduser(username, password=password, shell=shell)
126
127 self.assertEqual(result, new_user_pwnam)
128 check_call.assert_called_with([
129@@ -603,7 +603,7 @@
130
131 getpwnam.side_effect = [existing_user_pwnam, new_user_pwnam]
132
133- result = host.adduser(username, password,
134+ result = host.adduser(username, password=password,
135 primary_group='foo', secondary_groups=[
136 'bar', 'qux',
137 ])
138@@ -642,6 +642,52 @@
139 getpwnam.assert_called_with(username)
140
141 @patch('pwd.getpwnam')
142+ @patch('pwd.getpwuid')
143+ @patch('grp.getgrnam')
144+ @patch('subprocess.check_call')
145+ @patch.object(host, 'log')
146+ def test_add_user_uid(self, log, check_call, getgrnam, getpwuid, getpwnam):
147+ user_name = 'james'
148+ user_id = 1111
149+ uid_key_error = KeyError('user not found')
150+ getpwuid.side_effect = uid_key_error
151+ host.adduser(user_name, uid=user_id)
152+
153+ check_call.assert_called_with([
154+ 'useradd',
155+ '--uid',
156+ str(user_id),
157+ '--system',
158+ '-g',
159+ user_name,
160+ user_name
161+ ])
162+ getpwnam.assert_called_with(user_name)
163+ getpwuid.assert_called_with(user_id)
164+
165+ @patch('grp.getgrnam')
166+ @patch('grp.getgrgid')
167+ @patch('subprocess.check_call')
168+ @patch.object(host, 'log')
169+ def test_add_group_gid(self, log, check_call, getgrgid, getgrnam):
170+ group_name = 'darkhorse'
171+ group_id = 1005
172+ existing_group_gid = KeyError('group not found')
173+ new_group_gid = 1006
174+ getgrgid.side_effect = [existing_group_gid, new_group_gid]
175+
176+ host.add_group(group_name, gid=group_id)
177+ check_call.assert_called_with([
178+ 'addgroup',
179+ '--gid',
180+ str(group_id),
181+ '--group',
182+ group_name
183+ ])
184+ getgrgid.assert_called_with(group_id)
185+ getgrnam.assert_called_with(group_name)
186+
187+ @patch('pwd.getpwnam')
188 def test_user_exists_true(self, getpwnam):
189 getpwnam.side_effect = 'pw info'
190 self.assertTrue(host.user_exists('bob'))

Subscribers

People subscribed via source and target branches