Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add diff support to user module #48880

Open
wants to merge 1 commit into
base: devel
Choose a base branch
from
Open

Add diff support to user module #48880

wants to merge 1 commit into from

Conversation

mgedmin
Copy link
Contributor

@mgedmin mgedmin commented Nov 19, 2018

SUMMARY

Make the user module report what was changed if you run ansible-playbook --diff.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

user

ADDITIONAL INFORMATION

This was motivated by me trying to manage user group memberships with Ansible and having no way to verify that my playbook is correct when the user module reports changed with no details whatsoever.

What this does is produce brief diffs that lists old and new values of all user attributes that were modified, e.g.

--- before
+++ after
-home = /old/home/directory
-groups = list,of,old,groups
+home = /new/home/directory
+groups = list,of,new,groups

This is not 100% complete (only the generic usermod-based User implementation reports changes, all those FreeBsdUser etc subclasses need to be updated), but it's better than nothing.

When a user is created or deleted, instead of listing all the attributes only a brief summary is shown, e.g.

--- before
+++ after
-user does not exist
+user created

@ansibot
Copy link
Contributor

ansibot commented Nov 19, 2018

Hi @mgedmin, thank you for submitting this pull-request!

click here for bot help

@ansibot
Copy link
Contributor

ansibot commented Nov 19, 2018

@ansibot ansibot added affects_2.8 core_review feature module needs_triage support:core labels Nov 19, 2018
@bcoca bcoca removed the needs_triage label Nov 20, 2018
@sivel sivel requested a review from samdoran Nov 20, 2018
@samdoran samdoran changed the title user: support --diff Add diff support to user module Nov 20, 2018
@samdoran samdoran self-assigned this Nov 20, 2018
@ansibot ansibot added needs_rebase needs_revision and removed core_review labels Nov 20, 2018
This is not 100% complete (only the generic usermod-based User
implementation reports changes), but it's better than nothing.

Motivated by complete opaqueness of changed=True when a user's groups do
not match what Ansible wants them to be.
@ansibot ansibot added core_review and removed needs_rebase needs_revision labels Nov 22, 2018
Copy link
Contributor

@samdoran samdoran left a comment

This is a good start, but it does need some refinement. Please add support for other platforms as well as integration tests. Feel free to ask questions and I'll help in any way I can.

@@ -2609,6 +2629,10 @@ def main():
module.fail_json(name=user.name, msg=err, rc=rc)
result['force'] = user.force
result['remove'] = user.remove
result['diff'] = {
'before': 'user exists\n',
Copy link
Contributor

@samdoran samdoran Nov 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Return the actual username here:

Suggested change
'before': 'user exists\n',
'before': '{0} exists\n'.format(user.name),

@@ -2609,6 +2629,10 @@ def main():
module.fail_json(name=user.name, msg=err, rc=rc)
result['force'] = user.force
result['remove'] = user.remove
result['diff'] = {
'before': 'user exists\n',
'after': 'user removed\n',
Copy link
Contributor

@samdoran samdoran Nov 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'after': 'user removed\n',
'after': '{0} removed\n'.format(user.name),

@@ -2619,11 +2643,27 @@ def main():
else:
result['system'] = user.system
result['create_home'] = user.create_home
result['diff'] = {
Copy link
Contributor

@samdoran samdoran Nov 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this up to before line 2641 so that the diff is returned in check mode.

@@ -2619,11 +2643,27 @@ def main():
else:
result['system'] = user.system
result['create_home'] = user.create_home
result['diff'] = {
'before': 'user does not exist\n',
Copy link
Contributor

@samdoran samdoran Nov 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'before': 'user does not exist\n',
'before': '{0} does not exist\n'.format(user.name),

@@ -2619,11 +2643,27 @@ def main():
else:
result['system'] = user.system
result['create_home'] = user.create_home
result['diff'] = {
'before': 'user does not exist\n',
'after': 'user created\n',
Copy link
Contributor

@samdoran samdoran Nov 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'after': 'user created\n',
'after': '{0} created\n'.format(user.name),

@@ -744,17 +760,21 @@ def modify_user_usermod(self):
if current_expires < 0 or current_expire_date[:3] != self.expires[:3]:
cmd.append('-e')
cmd.append(time.strftime(self.DATE_FORMAT, self.expires))
self.changes['expires'] = (current_expires, self.expires)
Copy link
Contributor

@samdoran samdoran Nov 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Format the date string here so it's more human readable in the diff:

Suggested change
self.changes['expires'] = (current_expires, self.expires)
self.changes['expires'] = (current_expires, time.strftime(self.DATE_FORMAT, self.expires))

Copy link
Contributor Author

@mgedmin mgedmin Nov 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case I think I should also format current_expires the same way.

Copy link
Contributor

@samdoran samdoran left a comment

You'll need to return the diff when exiting in check mode:

module.exit_json(changed=True, diff=result['diff'])

@ansibot ansibot added needs_revision and removed core_review labels Nov 27, 2018
@ansibot ansibot added the stale_ci label Dec 6, 2018
@ansibot ansibot added the system label Feb 17, 2019
@ansibot ansibot added core_review and removed needs_revision labels Mar 3, 2020
@ansibot ansibot added needs_revision and removed core_review labels Mar 27, 2020
@ansibot ansibot added the needs_rebase label May 16, 2020
@ansibot ansibot added the needs_repo label Nov 3, 2020
@ansibot ansibot added pre_azp and removed stale_ci labels Dec 6, 2020
@samdoran samdoran removed their assignment Jan 7, 2021
@bcoca
Copy link
Member

bcoca commented Feb 16, 2022

waiting_on_contributor

@ansibot ansibot added the waiting_on_contributor label May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects_2.8 feature module needs_rebase needs_repo needs_revision pre_azp support:core system waiting_on_contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants