LGTM but every time I say that @firstyear and @spichugi have concerns. So lets see what they have to say...

How does "member attr" change between 1.3.7 given that member and memberof are reverse directions for membership?

instead of this helper function, you can use "obj.ensure_removed()" which does the state check and delete for you.

Is there a valid case where the user wouldn't exist and you try to rename them? This seems like a potential flaw in the test architecture if you "best effort" rename. Either you rename or don't I think.

This certainly will fail with the MEMBER_ATTR definition you have above on older versions. It's always been "member". ....

As a helper you can just use "group.list_members()" instead, and it will "do the right thing" for you.

Why do you call a disable before the enable? Just call enable()....

You probably don't want this timeout in restart, because highly-loaded CI servers will probably take longer than 10 seconds ....

Please provided a commented example of the "line you are matching", so that we can see what the regex is intending to find.

For the most part, pretty clean and nice use of the api I think. Some improvements, but pretty good, thanks.

@firstyear Thanks! I will correct the test with suggested changes.

Also questions:
1. So basically there is no need to distinguish member and memberOf between versions as these terms mean reverse directions for membership and I should keep the definition just to MEMBER_ATTR = 'memberOf'?
I found this function with versions in tests/suites/plugins/acceptance_test.py so I thought there was a change in that attribute.
2. What would be the sufficient timeout for restart? 60? Because 120 seems a bit too much although not impossible.

Hey there.

In this case, you are using the "group has a member which is user.dn". The reverse is "the user is a memberOf a group". So in this case, MEMBER_ATTR is "member".

If that's in acceptance_test.py, that may be a bug :|

Don't list the timeout at all. The function has a default value (which I think is 120). I'm not sure of your python experience, but python supports default args where if you provide no value, the default is used. :)

@firstyear
Thanks :)
Also I looked at the obj.ensure_removed() but what would be the correct way to use it? Because if I call it like “test_user.ensure_removed('uid','new_test_user_777')” then I get error “TypeError: 'NoneType' object is not callable”.
I thought it’s on the same level as obj.ensure_present() which works fine.

@bsmejkal The ensure_removed works on an instance of something. An example could be:

users = UserAccounts(standalone, DEFAULT_SUFFIX)
user = users.get('firstyear')
user.ensure_removed()

It doesn't need any arguments. The idea is you could have a pattern like:

user = UserAccount(standalone, <dn of user>)
user.ensure_removed()

So you don't know if the user existed, but you can blind create the object that "could" be there, then ensure it's removed. The initialisation of the DSLdapObject does not checks on ldap so it's safe to do this :)

@firstyear
I tried what you suggested on rhel8
(389-ds-base-1.4.0.20-7.module+el8+2809+aaa18b6a.x86_64) but if I try this:

users = UserAccounts(topology_st.standalone, DEFAULT_SUFFIX)
user = users.create_test_user(uid=777)
user.ensure_removed()

or this pattern:

user = UserAccount(standalone, )
user.ensure_removed()

I will still get error to provide those two arguments:

TypeError: ensure_removed() missing 2 required positional arguments: 'attr' and 'value'

So I tried:

user.ensure_removed('dn', user.dn)

then it passes, but user dn still won’t get removed (I checked) and the next test will fail, because that entry already exists.

Ahhhhhh, it turns out i'm the problem, and I misread the api. ensure_removed makes sure an attr:value is missing, not that the entry is deleted. Opps! This is my mistake for telling you the wrong thing.

The api for "ensure deleted" doesn't exist today, so ignore this comment and do the "if user.exists(): user.delete()" in that case. So sorry for this.

@firstyear
No problem :)
I made the suggested changes, could you please take a look? Thanks!

Thanks @bsmejkal This looks much better. Great work!

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3308

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.