(PUP-9562) User resource should respect forcelocal for the comment parameter #7768
Conversation
|
CLA signed by all contributors. |
GabrielNagy
force-pushed
the
PUP-9562/forcelocal-comment-fix
branch
from
3bc941d
to
8d4634c
Compare
A user resource configured with forcelocal will still try to sync the comment with the external directory services. It does use `lusermod` to modify the local `/etc/passwd` to the comment specified in the user resource, but it compares the `in_sync` with the external directory services, meaning that it always updates the comment on catalog compilation. This commit queries the comment from the local `/etc/passwd` if the `forcelocal` parameter is set. It also modifies the `finduser` method to return a hash of parameter/value type, since returning numbered elements is more error-prone.
GabrielNagy
force-pushed
the
PUP-9562/forcelocal-comment-fix
branch
2 times, most recently
from
986f3c2
to
83da627
Compare
| user = line.split(":") | ||
| if user[index] == value | ||
| return Hash[passwd_keys.zip(user)] | ||
| end |
There was a problem hiding this comment.
The old and new code read /etc/passwd once per user resource per managed parameter (until it finds a match). Can we not do that (in a future ticket/PR)? Seems like we could read /etc/passwd once per user resource without too much difficulty. Reading it once per transaction is also a possibility but users could be created mid-transaction...
There was a problem hiding this comment.
Good catch. Filed PUP-10117 for this issue.
| allow(provider).to receive(:finduser).with('account', 'myuser').and_return(passwd_line) | ||
| expect(provider).to receive(:localcomment).and_return('local comment') | ||
| provider.comment | ||
| allow(provider).to receive(:finduser).with(:account, 'myuser').and_return(user_object) |
There was a problem hiding this comment.
stubbing the finduser method is a code smell since it's part of the code that we're trying to test. It would be better to stub what File.open returns (like you do on line 339)
There was a problem hiding this comment.
I thought it'd be cleaner to stub finduser here since I'm already testing it separately below, and its "usage" is not really in the scope of the comment method which I'm testing here. Is this the way to go?
There was a problem hiding this comment.
There are lots of tests in puppet that do what you're describing, but I've been bitten by overmocking/stubbing so much that I avoid it as much as possible. For example, finduser is effectively a private method, so if you have tests like this, then refactoring in the future is harder.
There was a problem hiding this comment.
Makes sense, I changed it.
GabrielNagy
force-pushed
the
PUP-9562/forcelocal-comment-fix
branch
from
83da627
to
7808d91
Compare
This commit changes the `passwd_keys` array elements to be symbols instead of strings (since we do not manipulate them in the code), and updates the tests accordingly. Also, we do not explictly close the file since Ruby should do it for us when leaving the block's context. This commit also adds tests for `useradd#finduser` and fixes up some wrong expectations.
A user resource configured with
forcelocalwill still try to sync the comment with the external directory services. It does uselusermodto modify the local/etc/passwdto the comment specified in the user resource, but it compares thein_syncwith the external directoryservices, meaning that it always updates the comment on catalog compilation.
This commit queries the comment from the local
/etc/passwdif theforcelocalparameter is set. It also modifies thefindusermethod to return a hash of parameter/value type, since returning numbered elements is more error-prone.