I think the idea of this a lot. The dirsrv object should be the source of truth because it can access cn=config, so this makes a lot of sense.

If you don't mind, just a pseudo code example, how do you think this would look to use the module? I don't expect a full patch, but something you have in mind to help me clarify. I'm about 80% sure that I agree with you right now about the topic though :)

Sorry, I went mad and wrote the full patch.
(Not exactly full though. I'll add a patch for 389-ds-base tests before the pushing it).

45 +ENV_SYSCONFIG_DIR = '/etc/sysconfig'

Shouldn't this be in paths?

Otherwise I really like the look of this, it seems much nicer than the hardcoded constants we have.

``` 127 + instance_data = generate_ds_params(inst_num, role)
128 + if DEBUGGING:
129 + instance = DirSrv(verbose=True)
130 + else:
131 + instance = DirSrv(verbose=False)
132 + args_instance[SER_HOST] = instance_data[SER_HOST]
133 + args_instance[SER_PORT] = instance_data[SER_PORT]
134 + args_instance[SER_SERVERID_PROP] = instance_data[SER_SERVERID_PROP]
135 + args_instance[SER_CREATION_SUFFIX] = DEFAULT_SUFFIX
136 + args_copied = args_instance.copy()
137 + instance.allocate(args_copied)
138 + instance_exists = instance.exists()

can't we just update instance_data SUFFIX instead of copying all these? I think you could then pass instance data into allocate. Another option is allow generate_ds to just have a SUFFIX by default, because we never really change this, and then we can do;

instance.allocate(generate_ds_param(1, standalone))

As an example. 

Another option is to shortcut this, and just have:

Or some other name than allocate_qe :P

instance.allocate_qe(1, standalone)
```

and have that do the heavy lifting, without the need to pass the data around.

@firstyear thanks! Good thoughts. :)
I've changed path (basically I've used it in another implementation and I don't need it anymore, so I'd just comment it back)

Regarding instance.allocate, we have at lease two issues here that we need to find the most adequate way to solve or postpone it.
generate_ds_param gives REPLICA_ID also. So we either can create a new DirSrv.replica_id parameter in the object and interconnect it with Replica module. Or we can just return it within dictionary and use it like this:

instance_data = generate_ds_params(inst_num, role)
instance.allocate(instance_data)

if role in (REPLICAROLE_MASTER, REPLICAROLE_HUB, REPLICAROLE_CONSUMER):
    replicas = Replicas(instance)
    replica = replicas.enable(DEFAULT_SUFFIX, role, instance_data[REPLICA_ID])

But it is a small issue. The bigger one we have with SECURE_PORT part.
If we will do like in the example above we will give SER_SECURE_PORT to instance.allocate() that will result in self.sslport = args.get(SER_SECURE_PORT). And after this, it will mess the instance creation.
I think it will be the part of the ticket you assign to (#84)

So I propose to leave the version I have now:

instance_data = generate_ds_params(inst_num, role)
args_instance[SER_HOST] = instance_data[SER_HOST]
args_instance[SER_PORT] = instance_data[SER_PORT]
args_instance[SER_SERVERID_PROP] = instance_data[SER_SERVERID_PROP]
args_instance[SER_CREATION_SUFFIX] = DEFAULT_SUFFIX
args_copied = args_instance.copy()
instance.allocate(args_copied)

After you review the rest and agree to set 'ack', I will push it (I already made a change for "ENV_SYSCONFIG_DIR = '/etc/sysconfig'") and we will deal with the SSL issue later in another patch. What do you think?

Yep, I agree, I'm happy for you to defer the TLS issue to me, but let's add a comment in there about it.

With /etc/sysconfig, this should be from the ds paths module. It's paths.initconfig_dir . And this does matter because initconfig_dir = /opt/dirsrv/etc/sysconfig is what I have, and that's how we find instance locations. So lets use this properly :)

Yep, I agree, I'm happy for you to defer the TLS issue to me, but let's add a comment in there about it.

Sure, I've added it in 'create_topology()' function.

With /etc/sysconfig, this should be from the ds paths module. It's paths.initconfig_dir . And this does matter because initconfig_dir = /opt/dirsrv/etc/sysconfig is what I have, and that's how we find instance locations. So lets use this properly :)

Sorry, my English probably is not the best. I understand what you mean. And I don't need '/etc/sysconfig' anymore, so I've put all to the way it was.

Perfect. Great work!

Replace "masterX_agmts" with instance.agreement object.

Replace "masterX_agmts" with instance.agreement object.

ack

To ssh://pagure.io/lib389.git

a9f53cc..2edb3ce master -> master
commit 2edb3ce
Author: Simon Pichugin spichugi@redhat.com
Date: Mon Jul 31 09:58:25 2017 +0200

To ssh://pagure.io/389-ds-base.git

708938e37..e710054f6 master -> master
commit e710054f63f313fec20cacde8b78b105a8a211d8
Author: Simon Pichugin spichugi@redhat.com
Date: Wed Aug 9 17:17:34 2017 +0200

I'd say, we wait and see everyone is happy with a new change (also pointing the new way of writing in the review). And after some time we can go ahead and clean up all HOST_MASTER_4 constants from lib389 and 389-ds-base. I think it is manageable with IDE (or 'sed') and small manual work.

Also, I will prepare a patch with reworked create_test.py soon (probably tomorrow).

To ssh://pagure.io/389-ds-base.git

88fc5c026..63ea19dcb master -> master
commit 63ea19dcbe1cc979799cfc5d6cf0c600d453e3b8
remote: * Author: Simon Pichugin spichugi@redhat.com
remote: * Date: Thu Aug 10 12:34:32 2017 +0200

Fix regression in create_test.py

786b00e..3919217 master -> master