This name is odd. "refer-init" specificity the "init" part is not correct. Perhaps "refer-int"?

typo, should this be: _add_parser_args_definition

Should this be a boolean, action store true? Or choices=['yes', 'no']?

So the main issue if the naming you used for the RI plugin. The function, variable, and file names should be changed to not use refer-init,(or referinit). The "init" part is wrong and should be "int" or something more verbose.

Everything else looks pretty good, I'll wrap up my review later today.

You can better supply these to ldap.dn.dn2str which takes List[List[Tuple]].

I think the others commented here, but I use referint as shorthand, for anything external it should be referrential-integrity.

If you move this up, then if a _dn is provided, it's wiped out if we generate content from the tdn based on other parameters. So I'm not sure about this change.

I think this is enabling laziness, and I don't really like it. LDAP rdns are "based on the content of the entry", and by having it so that we can "have the rdn influcence the content of the entry" I think we are undoing some of that modeling.

I think I would ask what issue this is attempting to solve, because I think this change may be reflective of a problem elsewhere.

Why are you removing this comment?

Why are you removing this?

I'm okay with this change, but Marc probably needs to know for docs ...

I don't like the use of the word "edit" here because I was reserving that for an interactive entry editor I was plannig to add. Could this actually be something else?

Did you want to add the "modify" subcommand too?

Tests for the new dna plugin config types?

Part of me wonders if this could actually be split to about 3 seperate patches (new plugin functionality, new cli functionality, new js integration). It's a pretty long patch ...

This name is odd. "refer-init" specificity the "init" part is not correct. Perhaps "refer-int"?

I am okay with it if nobody else doesn't mind.

I think this is enabling laziness, and I don't really like it. LDAP rdns are "based on the content of the entry", and by having it so that we can "have the rdn influcence the content of the entry" I think we are undoing some of that modeling.
I think I would ask what issue this is attempting to solve, because I think this change may be reflective of a problem elsewhere.

What I want to have is to be able to create an entry without specifying RDN in the parameters and take it from DN:

properties = {"specattrname": "acctPolicySubentry",
              "limitattrname": "accountInactivityLimit"}
config = AccountPolicyConfig(inst, targetdn)
config.create(properties)

As far as I understand, DN should always contain RDN which is part of the entry attributes.
We already generate DN out of RDN and Base DN. Why not to get RDN from DN? It will make some operations easier (like here, in CLI, when a user provides DN for the new entry)
Do I miss something?

I don't like the use of the word "edit" here because I was reserving that for an interactive entry editor I was plannig to add. Could this actually be something else?

I don't see any problem. I will change it to set word and it will still be accurate.

Did you want to add the "modify" subcommand too?

Nope, I think the set of commands fulfill the main needs.

Tests for the new dna plugin config types?

Sure, I'll add it.

Why are you removing this?

I don't see any purpose why it should exist for now.
I've added generic plugin manipulations (plugin get, show, enable, disable) so the WhoAmI plugin can be accessed here.
Another reason why I don't see the sense of its existence as a separate plugin, - this special list of plugins is really just for the plugins that require some deep configuration. We don't have the documentation for the plugin too.

Why are you removing this comment?

Actually, I removed it because of the thing I explained at the beginning here (about RDN).

Part of me wonders if this could actually be split to about 3 seperate patches (new plugin functionality, new cli functionality, new js integration). It's a pretty long patch ...

There will be a separate PR for Cockpit.
And the new plugin functionality is just 50-100 lines of code (and the rest is over the thousand).
I think it makes sense to keep it here because this new plugin functionality is really tightened up with the rest of PR content.

This name is odd. "refer-init" specificity the "init" part is not correct. Perhaps "refer-int"?

I am okay with it if nobody else doesn't mind.

I think this is enabling laziness, and I don't really like it. LDAP rdns are "based on the content of the entry", and by having it so that we can "have the rdn influcence the content of the entry" I think we are undoing some of that modeling.
I think I would ask what issue this is attempting to solve, because I think this change may be reflective of a problem elsewhere.

What I want to have is to be able to create an entry without specifying RDN in the parameters and take it from DN:
properties = {"specattrname": "acctPolicySubentry",
"limitattrname": "accountInactivityLimit"}
config = AccountPolicyConfig(inst, targetdn)
config.create(properties)

This behaviour already exsits. I think your code is doing the opposite. It's doing:

config AcctPol(inst, 'cn=something,....'
config.create(properties={....})

Then cn is inferred into properties.

But I think the way you changed the code will break static dn and dn-derivation from the properties.

There is already a mechanism for inferring the RDN from the properties, so why not go that way instead? IIRC you give the type a basedn, but no rdn, and then TDN is built from basedn + inferred rdn from properties.

As far as I understand, DN should always contain RDN which is part of the entry attributes.
We already generate DN out of RDN and Base DN. Why not to get RDN from DN? It will make some operations easier (like here, in CLI, when a user provides DN for the new entry)
Do I miss something?

I don't like the use of the word "edit" here because I was reserving that for an interactive entry editor I was plannig to add. Could this actually be something else?

I don't see any problem. I will change it to set word and it will still be accurate.

Thank you !!!!

Tests for the new dna plugin config types?

Sure, I'll add it.

Great!

Why are you removing this?

I don't see any purpose why it should exist for now.
I've added generic plugin manipulations (plugin get, show, enable, disable) so the WhoAmI plugin can be accessed here.
Another reason why I don't see the sense of its existence as a separate plugin, - this special list of plugins is really just for the plugins that require some deep configuration. We don't have the documentation for the plugin too.

Ahhh, sure. Just wanted to clarify.

Why are you removing this comment?

Actually, I removed it because of the thing I explained at the beginning here (about RDN).

Part of me wonders if this could actually be split to about 3 seperate patches (new plugin functionality, new cli functionality, new js integration). It's a pretty long patch ...

There will be a separate PR for Cockpit.
And the new plugin functionality is just 50-100 lines of code (and the rest is over the thousand).
I think it makes sense to keep it here because this new plugin functionality is really tightened up with the rest of PR content.

Sure,

This behaviour already exsits. I think your code is doing the opposite. It's doing:
config AcctPol(inst, 'cn=something,....'
config.create(properties={....})
Then cn is inferred into properties.
But I think the way you changed the code will break static dn and dn-derivation from the properties.

I don't see how it breaks static DN and DN-derivation... I checked some test suites and lib389 performs the same way after my previous change... But I moved the code to generic_object_add function so we can get rid of additional magic and make it more transparent.

There is already a mechanism for inferring the RDN from the properties, so why not go that way instead? IIRC you give the type a basedn, but no rdn, and then TDN is built from basedn + inferred rdn from properties.

I wanted exactly the oposite. I want to substruct the RDN from DN and put it to the properties.
Check what I've done in generic_object_add and then check how I use it for account-policy (and other plugins).

Tests for the new dna plugin config types?
Sure, I'll add it.

Great!

I've ported the only test we have for the DNA Shared Config. It is simple but it exposes basic functionality of the object. More deep multi-master topology testing will require much more time. So I think the current test is enough for the basic purpose (and we have one more test ported to suites+DSLdapObject - yay!).

Okay, better idea. I'm going to write and submit a "dn construction test" that asserts all the various ways we build dn's for objects today, and if that passes, we go ahead? That way we can assert everything works as we want :)

https://pagure.io/389-ds-base/pull-request/50267

So I have noticed something INTERESTING writing these tests. It looks like what you want todo (allow properties to be derived from the rdn value) already works provided the rdn=X and rdn: Y is in properties. It's actually a lib389 check that blocks the missing rdn in properties case, (commented in the test).

I think this is also a feature of 389-ds, it doesn't look like it's a lib389 behaviour because the logs clearly show we are only sending that single property.

So I'd be curious to see these tests run with your patch as well, and if it corrects that case.

The tests pass! Thank you!
Please, check once again because all of the comments were incorporated, I think.

Seems okay to me now I think, I think that the dn construction tests in 50267 may need to change after this patch though. Anyway ack from me. Any comments @mreynolds ?

LGTM

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/3318

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.