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.

Sign up for GitHub

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

Jump to bottom

Add automember module #486

Merged
merged 1 commit into from May 26, 2021
Merged

Add automember module #486

merged 1 commit into from May 26, 2021

Conversation

jake2184
Copy link

@jake2184 jake2184 commented Jan 7, 2021

I've extended & updated an automember module, first proposed in #392. That PR looked active for a couple of weeks, but silent for months. I've forked it, rebased on master, and addressed a few of the issues highlighted in the PR.

I can't tag the previous author, Mark Hahl / @wolskie. I haven't changed any of the author notes or copyright.

@jake2184
Copy link
Author

jake2184 commented Jan 7, 2021

oh, the tag did work! Just didn't autocomplete.

@rjeffman
Copy link
Member

Hello @jake2184,

The task failing is due to differences in string handling in Python 2 and 3. To fix this issue you should add the following to the imports:

from ansible.module_utils._text import to_text

And then, when you need the string, you use to_text(data)

Thank you for you contribution!

@jake2184 jake2184 force-pushed the master branch 2 times, most recently from 9535f38 to f9cb369 Compare January 13, 2021 11:09
@jake2184
Copy link
Author

I seem to be getting errors now that I'm fairly sure are unrelated to any of my changes:

> {"ansible_facts": {"discovered_interpreter_python": "/usr/libexec/platform-python"}, "changed": false, "msg": "Kerberos authentication failed: kinit: Cannot read password while getting initial credentials\n"

This is during facts collection? Is there anything I need to setup (rebase on master?) or change to get these working?

@rjeffman
Copy link
Member

That is really weird... I'll look into that in the next days, as I really don't have the time now.

rjeffman
rjeffman requested changes Jan 13, 2021
View reviewed changes
playbooks/automember/automember-absent.yml Outdated Show resolved Hide resolved
playbooks/automember/automember-present.yml Outdated Show resolved Hide resolved
plugins/modules/ipaautomember.py Outdated Show resolved Hide resolved
plugins/modules/ipaautomember.py Outdated Show resolved Hide resolved
plugins/modules/ipaautomember.py Show resolved Hide resolved
tests/automember/test_automember.yml Outdated Show resolved Hide resolved
This was referenced Jan 13, 2021
Closed
Closed
@rjeffman
Copy link
Member

@jake2184 the failures on the checks, currently, have nothing to do with your changes as we are having some issues with the images used for testing. I'm fixing those, but we can't use the checks for a while. I'll let you know when everything is working again.

jake2184 added a commit to jake2184/ansible-freeipa that referenced this pull request Jan 19, 2021
@jake2184
ipaautomember: Resolve comments from review on freeipa#486
71d2a48
jake2184 added a commit to jake2184/ansible-freeipa that referenced this pull request Jan 19, 2021
@jake2184
ipaautomember: Resolve comments from review on freeipa#486
1414d72
jake2184 added a commit to jake2184/ansible-freeipa that referenced this pull request Jan 19, 2021
@jake2184
ipaautomember: Resolve comments from review on freeipa#486
9b9f8e6
@rjeffman
Copy link
Member

@chr15p, all test environments are up and running again. Please, fix the failing tests.

@jake2184
Copy link
Author

jake2184 commented Feb 1, 2021

Tests running cleanly

@rjeffman
Copy link
Member

rjeffman commented Feb 1, 2021

@jake2184 great job!

I'll try to do a proper review during this week.

@jake2184
Copy link
Author

Can I bump this please @rjeffman ?

@jake2184
Copy link
Author

jake2184 commented Mar 2, 2021

And again @rjeffman @t-woerner

@rjeffman
Copy link
Member

rjeffman commented Mar 3, 2021

@jake2184 I was on vacation, and only returning "tomorrow".

I'll look at it Really Soon Now (tm). :-)

@jake2184
Copy link
Author

jake2184 commented Mar 3, 2021

No worries, as long as it doesn't fall through the cracks :)

@jake2184
Copy link
Author

Is this going to be looked at any time soon? @rjeffman

@rjeffman
Copy link
Member

Hello, sorry for the too long delay.

We're having issues with CI (and available time), as soon as CI is fixed, we'll restart pushing PRs, automount and automember modules are a priority.

I hope this to be soon, but can't promise as, right now, time is at a premium.

@rjeffman
Copy link
Member

rjeffman commented May 3, 2021

Hello @jake2184,

We had issues with CI, and the fixes require that all pending PR are rebased.

Can you please rebase your PR?

@rjeffman
Copy link
Member

Tested on the CLI (not the module) and it should work with Centos 7. Just restarted the test.

All FreeIPA version are different in the tested environments. CentOS 7 has FreeIPA version 4.6.8, CentOS 8 is 4.8.7 and Fedora is 4.9.3.

@rjeffman
Copy link
Member

After re-running the tests, the same test still fails. Fixing it will require some debuging.

@rjeffman
Copy link
Member

I could reproduce your issue locally, with CentOS-7, but have no time to debug it.

If you have podman installed, use this script to test in a container and debug (usually, I debug with ansible_module.warn("VAR: %s" % var)).

@rjeffman
Copy link
Member

For some reason, it looks like key and automemberinclusiveregex are swaped. I didn't look any further.

TASK [Add testhostgroup hostgroup automember rule member condition] ************************************
[WARNING]: testhostgroup automember_add_condition: {'automemberinclusiveregex': u'fqdn', 'type':
u'hostgroup', 'key': u'.*.domain.com'}
fatal: [dev-test-master]: FAILED! => {"changed": false, "failed_when_result": true, "msg": "automember_add_condition: testhostgroup: .*.domain.com is not a valid attribute."}

@rjeffman rjeffman mentioned this pull request May 24, 2021
Closed
t-woerner added a commit to t-woerner/ansible-freeipa that referenced this pull request May 25, 2021
@t-woerner
ansible_freeipa_module.py: Add ignore argument to compare_args_ipa
f9b7091 
The new argument ignore has been added to compare_args_ipa to ignore
attributes while comparing attributes of the user args and the object
args returned from IPA.

This code is using changes from
- Wolskie in PR freeipa#392
- jake2184 in PR freeipa#486
@t-woerner t-woerner mentioned this pull request May 25, 2021
Merged
t-woerner added a commit to t-woerner/ansible-freeipa that referenced this pull request May 25, 2021
@t-woerner
ansible_freeipa_module.py: Add ignore argument to compare_args_ipa
b1c1615 
The new argument ignore has been added to compare_args_ipa to ignore
attributes while comparing attributes of the user args and the object
args returned from IPA find or show command.

This code is using changes from
- Wolskie in PR freeipa#392
- jake2184 in PR freeipa#486
@rjeffman
Copy link
Member

@jake2184, sorry, but you'll need to rebase this PR.

For plugins/module_utils/ansible_freeipa_module.py just use what's on the master branch.

jake2184 added a commit to jake2184/ansible-freeipa that referenced this pull request May 26, 2021
@jake2184
ipaautomember: Resolve comments from review on freeipa#486
c0ee4b8
jake2184 added a commit to jake2184/ansible-freeipa that referenced this pull request May 26, 2021
@jake2184
Update automember module after review - freeipa#486
094bcf3
jake2184 added a commit to jake2184/ansible-freeipa that referenced this pull request May 26, 2021
@jake2184
Add rebuild parameters to ipaautomember moduke - freeipa#486
717fdef
jake2184 added a commit to jake2184/ansible-freeipa that referenced this pull request May 26, 2021
@jake2184
Fix bug in automember where expression contained '=' - freeipa#486
00ec52d
jake2184 added a commit to jake2184/ansible-freeipa that referenced this pull request May 26, 2021
@jake2184
Update automember module after review - freeipa#486
f10017a
@jake2184
Copy link
Author

Rebased, comments resolved, awaiting CI checks

rjeffman
rjeffman previously approved these changes May 26, 2021
View reviewed changes
Copy link
Member

@rjeffman rjeffman left a comment

Choose a reason for hiding this comment

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

LGTM.

@rjeffman rjeffman dismissed their stale review May 26, 2021 16:35

Forgot that new modules must have a single commit.

@rjeffman
Copy link
Member

@jake2184, code for automember is good.

Please, merge all the commits into a single one, and use a commit message base on the one found on PR #357.

t-woerner
t-woerner reviewed May 26, 2021
View reviewed changes
README.md Show resolved Hide resolved
@jake2184
New automember management module
0e0bdf1 
    There is a new automember management module placed in the plugins folder:

        plugins/modules/ipaautomember.py

    The automember module allows to ensure presence or absence of automember rules
    and manage automember rule conditions.

    Here is the documentation for the module:

        README-automember.md

    New example playbooks have been added:

        playbooks/automember/automember-group-absent.yml
        playbooks/automember/automember-group-present.yml
        playbooks/automember/automember-hostgroup-absent.yml
        playbooks/automember/automember-hostgroup-present.yml
        playbooks/automember/automember-hostgroup-rule-absent.yml
        playbooks/automember/automember-hostgroup-rule-present.yml

    New tests for the module:

        tests/automember/test_automember.yml
t-woerner
t-woerner approved these changes May 26, 2021
View reviewed changes
Copy link
Member

@t-woerner t-woerner left a comment

Choose a reason for hiding this comment

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

LGTM

@t-woerner t-woerner merged commit f007c5c into freeipa:master May 26, 2021
@t-woerner t-woerner mentioned this pull request May 26, 2021
Closed
@mhahl
Copy link
Contributor

mhahl commented May 26, 2021

@jake2184 thank you for taking over the work on this module, I appreciate it very much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chr15p chr15p chr15p left review comments

@t-woerner t-woerner t-woerner approved these changes

@rjeffman rjeffman rjeffman left review comments

Assignees
No one assigned
Labels
needs-rebase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@jake2184 @rjeffman @t-woerner @mhahl @chr15p