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

New utils method that returns the lists of members. #1020

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

Conversation

dkarpele
Copy link
Collaborator

@dkarpele dkarpele commented Jan 12, 2023

Extend netgroup and sudorule modules to support external
users and hosts wherever possible.
Add tests for ipanetgroup and ipasudorule.

Problem statement:

    - name: Ensure sudorule is present with users and hosts (action member)
      ipasudorule:
        name: testrule2
        user:
          - external-user
        action: member
    - name: Ensure sudorule is present with users and hosts (action member) again
      ipasudorule:
        name: testrule2
        user:
          - external-user
        action: member

After execution of the first task with external users ansible returns changed as expected, after second it still returns changed - it's a bug. This PR fixes it. After the second task ansible will return ok.
"External" entities are:
for ipasudorule: externalhost, externaluser, ipasudorunasextuser, ipasudorunasextgroup
for ipanetgroup: externalhost

@dkarpele dkarpele changed the title WIP: New utils method that returns the lists of members. New utils method that returns the lists of members. Jan 13, 2023
@t-woerner
Copy link
Member

Please add information about externalhost and externaluser to the commit message and also as a comment to the code to make is easier to understand where these are coming from and what is returned in there.
Please also change the name of the function to be self-explaining. Something like concat_attr_list or similar to show that is concatenating lists.

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.

Please, add the new test playbooks to the "*_client_context" test playbook.

Client context test is important for any method dealing with lists as lists are returned as tuples in this case.

@rjeffman
Copy link
Member

About the client context tests, I'd like to have a single "test_*_client_context.yml" file per module, so you can simply add the regular test file as "import_playbook" to the existing client context test playbook.

As this might be debatable, @t-woerner, do you agree?

@dkarpele
Copy link
Collaborator Author

dkarpele commented Jan 25, 2023

About the client context tests, I'd like to have a single "test_*_client_context.yml" file per module, so you can simply add the regular test file as "import_playbook" to the existing client context test playbook.

As this might be debatable, @t-woerner, do you agree?

Thanks @rjeffman, it makes sense as for example there are already test_netgroup_member.yml and test_netgroup.yml tests in test_netgroup_client_context.yml. I update the PR.

@dkarpele dkarpele force-pushed the dkarpele-789 branch 2 times, most recently from 6680c05 to c3d7131 Compare January 30, 2023 14:16
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.

I really like the idiom allowed by concat_list_attr. Thank you for this!

@dkarpele dkarpele self-assigned this Feb 15, 2023
@dkarpele dkarpele force-pushed the dkarpele-789 branch 2 times, most recently from 93a56a6 to f56b689 Compare February 19, 2023 17:55
@dkarpele dkarpele requested a review from t-woerner February 19, 2023 17:55
Extend netgroup and sudorule modules to support external
users and hosts wherever possible.
Add tests for ipanetgroup and ipasudorule.

Problem statement:
```
    - name: Ensure sudorule is present with users and hosts (action member)
      ipasudorule:
        name: testrule2
        user:
          - external-user
        action: member
    - name: Ensure sudorule is present with users and hosts (action member) again
      ipasudorule:
        name: testrule2
        user:
          - external-user
        action: member
```
After execution of the first task with external users ansible
returns changed as expected, after second it still returns
changed - it's a bug. This PR fixes it. After the second task
ansible will return ok.
"External" entities are:
for `ipasudorule`: `externalhost, externaluser, ipasudorunasextuser,
ipasudorunasextgroup`
for `ipanetgroup`: `externalhost`

Signed-off-by: Denis Karpelevich <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants