-
Notifications
You must be signed in to change notification settings - Fork 247
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
Tests: Test transformation of bash-ldap-id-ldap-auth netgroup #7648
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you using a specially created function to run libc's group membership, when the framework already provides an automation to check this?
As an example test_netgroups__add_remove_netgroup_member
uses client.tools.getent.netgroup("ng-2")
to check the membership.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please reference current code for examples to better conform to the new coding standards. Let's pause any other PRs until this one meets the new guidelines.
f0c4c4e
to
50ad1c3
Compare
40b026b
to
a19c0cb
Compare
This is much better, but you resolved an issue that still needs to be resolved. These tests are specific to LDAP should go in test_ldap.py. Is there a reason why we cannot use the generic provider? We want to start using this for more coverage and reduce the amount of test cases. Not related to this PR, here is an example of using one test to cover the same scenario for AD/IPA/LDAP/Samba, 12 lines of code instead of copying the test case. https://github.com/SSSD/sssd/blob/master/src/tests/system/tests/test_failover.py I'll take a closer look later, all the checks need to be green before we merge, please resolve the ci failures, like the 'static code analysis' and the ci system tests. |
These are ldap specific tests. I have tries running them against GenericProvider but its failed . So moving them to _test_ldap.py |
78ca7f8
to
fe91a15
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall it looks good, mostly grammatical changes requested. I didn't go into why, the change, but if you would like to explain why moving forward I'm happy to do so or I can just provide the change.
There is one part of 'test_netgroup__lookup_nested_groups' that is out scope, thanks Anuj, good work.
for id in [1, 2]: | ||
provider.user(f"ng{id}").add() | ||
|
||
netgroup_qa = provider.netgroup("QAUsers").add() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should change the group names, it's more concise to call them "group" and "nested_group".
""" | ||
:title: Netgroup contains a member that has a host and domain specified. | ||
:setup: | ||
1. Create users, groups. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'Create users and groups.'
@pytest.mark.topology(KnownTopology.LDAP) | ||
@pytest.mark.topology(KnownTopology.AD) | ||
@pytest.mark.topology(KnownTopology.Samba) | ||
def test_netgroup__host_and_domain(client: Client, provider: GenericProvider, user: str, domain: str, expected: str): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A more descriptive name, like
'test_netgroup__lookup_nested_groups_with_host_and_domain_values_present'
""" | ||
:title: Nesting netgroups and verifying user memberships using LDAP with sssd. | ||
:setup: | ||
1. Create users, groups. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'Create users and groups.'
@pytest.mark.topology(KnownTopology.Samba) | ||
def test_netgroup__lookup_nested_groups(client: Client, provider: GenericProvider): | ||
""" | ||
:title: Nesting netgroups and verifying user memberships using LDAP with sssd. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'Looking up nested netgroups' is sufficient as the title.
5. Add Circular Netgroup Nesting to nested_netgroup | ||
6. Start sssd | ||
:steps: | ||
1. Retrieves all members of the "nested_netgroup" group using the getent netgroup tool. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'Lookup nested_netgroup'
We can drop steps 2 and 3.
3. Checks if a user who is not in any netgroup is part of "nested_netgroup". | ||
4. After the SSSD restart, it retrieves the members of "nested_netgroup" again to ensure they still intact. | ||
:expectedresults: | ||
1. All members of the "nested_netgroup" group be there |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Netgroup is found, and both netgroups and users are members"
We can drop all the other steps.
""" | ||
:title: User's 'memberNisNetgroup' attribute values are the DN of the group. | ||
:setup: | ||
1. Create users, groups. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a lot of comments to the following test case, all of those comments apply here.
""" | ||
:title: User's 'memberNisNetgroup' attribute values are the DN of the group. | ||
:setup: | ||
1. Create users, groups. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a lot of comments to the following test case, all of those comments apply here.
client: Client, provider: GenericProvider, operation: str | ||
): | ||
""" | ||
:title: User's 'memberNisNetgroup' attribute values are the DN of the group. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove . from all titles and steps. Let's try and make it consistent.
bd1e66d
to
abc7d8e
Compare
:setup: | ||
1. Create users and groups | ||
2. Create netgroups and add member | ||
3. Add Members to nested_netgroup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lower case M in members
:setup: | ||
1. Create users and groups | ||
2. Create netgroups and add member | ||
3. Start sssd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Start SSSD
:steps: | ||
1. Check whether the expected member is present in the nested_group netgroup | ||
:expectedresults: | ||
1. Member is present in the nested_group netgroup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is confusing, either
'Member is present in nested netgroup'
"Member is present in netgroup 'nested_netgroup'"
2. Create netgroups and add member | ||
3. Start sssd | ||
:steps: | ||
1. Check whether the expected member is present in the nested_group netgroup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'Lookup netgroup', the extra wording is not necessary.
4. Make netgroup and nested_netgroup members of one another, looping the groups | ||
5. Start sssd | ||
:steps: | ||
1. Lookup nested_netgroup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep names, in quotes,
- Lookup 'nested_netgroup'
:setup: | ||
1. Create users and groups | ||
2. Create netgroups and add member | ||
3. Add Members to nested_netgroup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quotes, if you are using the variable name.
- Add members to netgroup 'nested_netgroup'
or - Add members to nested netgroup
One specifies the actual group name and the other option is more general. I don't care which you choose, but I'd like to see it consistent through the file.
2. Create a new netgroup called group and add a member (ng1) to group | ||
3. Create another netgroup named nested_group and add a member (ng2) to nested_group | ||
4. Modify the nested_group to replace its members with the members of group | ||
5. Start sssd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uppercase SSSD
abc7d8e
to
2d7960d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a minor improvement. You should also check the python-system-tests
result since mypy
is reporting several errors.
84a504d
to
755b6e8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just the three minor typos, but approved. Thank you Anuij.
2. Create netgroups and add member | ||
3. Start SSSD | ||
:steps: | ||
1. Lookup netgroup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lookup netgroup "nested_group"
:title: Netgroup contains a member that has a host and domain specified | ||
:setup: | ||
1. Create users and groups | ||
2. Create netgroups and add member |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'members' plural
:title: Looking up nested netgroups | ||
:setup: | ||
1. Create users and groups | ||
2. Create netgroups and add member |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'members' plural
b217f20
to
7eb8b21
Compare
Before we move forward and merge this PR we need to fix the CI issues. python-system-tests report a set of issues regarding the static typing from On top of that, there are some problems related to polarion in system tests. You can check them in the following link: system (fedora-39). |
I do not see any of the error mentioned above, also test are running fine . IPA does not support domain in netgroups, therefore the generic interface does not have it. Due to this there may be some mypy error, which are false positive . ============================================================================================================ test session starts ============================================================================================================= Selected tests will use the following hosts: collected 15 items tests_local/netgroup.py ............... [100%] ======================================================================================================= 15 passed in 116.36s (0:01:56) ======================================================================================================= (.venv) aborah:tests: mypy test_netgroups.py
Polarion problem is not related to this PR . |
These are the problems reported:
I don't agree. This is the problem reported:
You added |
7eb8b21
to
008a8c6
Compare
IPA does not support domain in netgroups, therefore the generic interface does not have it. This is the reason for this error.
Above problem is fixed now . |
I've been investigating it and that's not the reason. This is the interface definition:
Link to source: https://github.com/SSSD/sssd-test-framework/blob/master/sssd_test_framework/roles/generic.py#L794 As you can see That's one of the failures, I'd recommend you to read the complete list and understand what's failing in each case.
Good! |
Domain is needed for the test . @pbrezina can help ?
|
fe556d0
to
1ddbe88
Compare
Test transformation of bash-ldap-id-ldap-auth netgroup
1ddbe88
to
572ac26
Compare
All Good now !! |
Test transformation of bash-ldap-id-ldap-auth netgroup