display_attr

Why did you choose to use this method?

It would be good witha function like this, to add tests - lib389/tests/, make a plugin directory and put a memberof test in. Yes, it's testing memberOf's functionality, but we need to test that setting these values actually influences the plugin correctly. It also checks the api is correct too :)

Hope that helps, great to see your first patch .

Hello William,

I was about to add tests just right after this, but I wanted to discuss first better what and how it will be tested. I'm submitting now a more complete patch that allows memberof to be configured through dsconf.

Why did you choose to use this method?

Why not? I want to display values in an "attr: val" format and this method does exactly this. Do you prefer some other format?

About testing. I need to act upon a dummy test instance. How will I initialise this instance? With what arguments?

Additionally, I want to discuss the following things about the patch I'm submitting:

1) All dsconf functions begin with the following line:

plugin = MemberOfPlugin(inst)

I wonder if we can avoid this repition.

2) The show(), enable(), disable() methods will be the same for all plugins, we just need to apply them on a different plugin object every time. Again, any easy way in order to not repeat ourselves?

3) I don't really like the set_or_display_x() function names. It's also obvious that these functions are violating the single responsibility principle.

4) MemberOfPlugin.set_autoaddoc() will allow any attribute for memberofautoaddoc. LDAP is not going to complain about it. However we know that only 3 values really support memberOf (inetAdmin, inetUser, nsMemberOf). Should MemberOfPlugin.set_autoaddoc() discard any other values except for these three since LDAP is not going to prevent the user from setting an "invalid" value? Btw, in the "upper layer" dsconf, I'm limiting the possible inputs from the user thanks to argparse, but I don't know if we should just rely on that.

Thanks.

Why not? I want to display values in an "attr: val" format and this method does exactly this. Do you prefer some other format?

I just wanted to know why you chose it, that's all :) I couldn't remember what it did, so I wanted to see what your thinking was.

About testing. I need to act upon a dummy test instance. How will I initialise this instance? With what arguments?

As in a test directory server instance? Look at the other tests in lib389, they use the "topology" code to build a throw-away instance.

Additionally, I want to discuss the following things about the patch I'm submitting:
1) All dsconf functions begin with the following line:
plugin = MemberOfPlugin(inst)

There is a way to add the MemberOf plugin as an attr on DirSrv, but it creates an import loop. Look at https://pagure.io/lib389/blob/master/f/lib389/init.py#_290

Another option might be to use a decorator to achieve this?

I think that we should think about the right solution here :) Perhaps there is a way based on the _generics code?

I wonder if we can avoid this repition.
2) The show(), enable(), disable() methods will be the same for all plugins, we just need to apply them on a different plugin object every time. Again, any easy way in order to not repeat ourselves?

Have a look at how I structured the generic* functions in the cli_conf/backend.py, and lib389/cli_base/init.py

3) I don't really like the set_or_display_x() function names. It's also obvious that these functions are violating the single responsibility principle.

Where are these functions?

4) MemberOfPlugin.set_autoaddoc() will allow any attribute for memberofautoaddoc. LDAP is not going to complain about it. However we know that only 3 values really support memberOf (inetAdmin, inetUser, nsMemberOf). Should MemberOfPlugin.set_autoaddoc() discard any other values except for these three since LDAP is not going to prevent the user from setting an "invalid" value? Btw, in the "upper layer" dsconf, I'm limiting the possible inputs from the user thanks to argparse, but I don't know if we should just rely on that.

This would be an issue in the C code - we should never enforce these limits in python, because someone can always edit the dse.ldif/cn=config without our interaction.

This is genuinely a bug, so if you want, raise this issue in 389-ds-base, and describe that the autoaddoc doesn't do the check. :)

Follow up from IRC, I think it would be good to test the CLI also. I linked you to the example of the backend tests for this.

Sure, I'll add the CLI tests promptly. In the meantime I'm uploading a new patch and I want your opinion on something.

As I said I want the show(), enable(), disable() methods to be generic for all plugins. So I moved these functions inside lib389/cli_conf/plugin.py and I'm passing the plugin's class through args, so they can be used to enable any plugin.

So,

1) Do you like this approach?

I named the above mentioned functions as generic_enable(), generic_disable() and generic_show(). The reason is that inside lib389/cli_conf/plugin.py there are already plugin_enable() and plugin_disable() functions, but they behave differently. They prompt the user for the dn of the plugin that they want to enable/disable.

2) Are we really going to need/use these latter functions?

3) If so, do you think that the generic_x() names are ok as names for the former functions?

Yeah, I like this. It works well, and easily, and we can re-use it.

The plugin_enable and plugin_disable are shims to emulate a legacy behaviour. We can remove them, but it means we need to check every test in ds if they were using them or not .....

Otherwise, I'm pretty happy with this in it's current form. It's a great start.

I have added the cli tests.

ack from me! Great work, thanks for your effort on this.

commit ede3dc03e9ae66e161efa6c3eb2b6a4d7cc5df20
To ssh://git@pagure.io/lib389.git
4b97c75..ede3dc0 master -> master

The first patch is for some forgotten stuff.
The second patch adds functionality tests and it depends on the first one.

In line 174 of the second patch I'm creating a new ou the "normal way".

I'm not sure if I should create a cn="ou=People2,dc=example,dc=com",cn=mapping tree,cn=config entry instead, since I'm storing it in a different backend.

In any case, the test case seems valid and executes properly as is now, but I would be happy to change it if it makes more sense.

Because you are using the instance.backends.create() api, it creates the mapping tree for you. That's why this is "just working" for you :)

commit f3991c25be6676289229f3852ae897c76eb23131
commit 65499d461d28f9debf1671b6a07c9fc99e3beb66
To ssh://git@pagure.io/lib389.git
ede3dc0..65499d4 master -> master

Last one, about fixup task:

commit 121ebdbeeef3656116eca9cfa8b9319d7463f5a9
To ssh://git@pagure.io/lib389.git
b28ef18..121ebdb master -> master

When you're ready, set the ticket to "closed: fixed" !

One final patch regarding this one:

I was waiting for #49284 to get fixed.

I have added a "delall" positional argument to delete all memberOfEntryScope or memberOfEntryScopeExcludeSubtree.

So the syntax is:

... memberof scope add "dc=example,dc=com"
... memberof scope add "dc=example2,dc=com"
... memberof scope del "dc=example,dc=com"
... memberof scope delall

I don't think I like the name "delall" that much, but I couldn't think of anything better. So if you have any suggestions, I would be happy to change it.

commit c7f4d7eb1d67fea57a1af9de647f483b859b9af5
To ssh://git@pagure.io/lib389.git
121ebdb..c7f4d7e master -> master