Hi Thierry,

before all sorry if in the rest of the answer I may seem too direct. All I say 
is obviously imh(opinion|experience) Time is a tyrant :(


On Friday 03 January 2014 16:36:17 thierry bordaz wrote:

    Thanks, I also wish you an happy new year and you recover well. It
    is great to talk to you again :-) .

Thx++!

    I am quite novice with python and my first approach was to use it as
    a real object oriented language,

It *is* a real OOL ;) 

it is a
    common use to wrap methods of class and rather use python as a
    functional language (e.g. self.backend.setProperties(...) rather
    than 'backend=Backend(); backend.setProperties(..) ')

I'm not indepth with functional programming. Why do you think that's a 
functional approach?

    ...the 'object'...are... the configuration object stored in
    'cn=config'. So it prevents the problem of synchronizing the python
    object with the 'cn=config' object.

To me that's mostly an ORM approach: in any case that's ok 

I will try to answer your concerns but as all of them are valid I will only give some reasons for the approach I choose.

About OOL vs. functional programing this is not IMH that important. For example for an instance with N replicas, we can imagine DSAdmin having N Replica/Backend/Suffix/MappingTree... objects. Instead we have only one, let's say Replica object, allocated in __add_brookers__ but this object is not a real object but just gives access all methods of  Replica class.
As I said, having N Replica objects brings a big drawback to synchronize the objects with what is in the server config. So I think the __add_brookers__ approach is better.

    So the only remaining object is
    the DS instance (dirsrv/dsadmin) and methods to access its config.

    Does it prevent to use fake directory ? I am not enough familiar
    with fakeldap, would you give me an example of why the current
    implement would not work with fakeldap ?

Let's see how do we setup the client now:
        args = {SER_HOST:          LOCALHOST,
                SER_PORT:          INSTANCE_PORT,
                SER_DEPLOYED_DIR:  INSTANCE_PREFIX,
                SER_SERVERID_PROP: INSTANCE_SERVERID
                }
        1- instance = DirSrv(verbose=False)
        2- instance.allocate(args)
        3- if instance.exists(): instance.delete()
        4- instance.create()
        5- instance.open()

That's quite different from the (imho cleaner) old approach - similar to the  
SimpleLDAPObject superclass:
	args = {'host': LOCALHOST, 'sslport': 10636}
	1- instance = DSAdmin(**args)

I agree with you DSAdmin approach looks definitely simpler. Now there is some magic behind DSAdmin().
It actually checks if the instance exists, if not it creates it, then it opens a connection to it.
What I wanted to do in a lib is to give an interface to each individual action. We may want to create an instance without establishing a connection to it, or (re)create an instance even if one already exists.
Your point is valid, we need something simple. So what about adding a new interface to DirSrv (e.g. NewAndConnect) that would do all the steps 1-5 ?

Obviously there are plenty of functionalities DSAdmin didn't implement: I 
would have put those functionalities (eg. filesystem related, instance 
creation & removal) in the DSAdminTool class.

You may ask: why having two class DSAdminTool and DSAdmin instead of just one?
1- clear design: as DSAdmin extends SimpleLDAPObject, it should require just a 
tcp connection (like SimpleLDAPObject). In this way I can use a mock ldap 
implementation to test the LDAP behavior of DSAdmin.

DirSrv also extends SimpleLDAPObject and wrap all its methods (search/add...) what would prevent it to be use as mock ldap ?

2- all the functionalities requiring filesystem access and instance management 
(eg. outside LDAP scope) couldn't be tested by a mock ldap implementation, so 
we can just extend the mock for those functionalities.

3- As extending classes (or using mix-ins) in python is really smooth, this 
approach would lead to the same usage patterns we have now, but with a 
separate, tcp oriented DSAdmin class.

Yes that looks very smart. Couldn't it been done with DirSrv that in my understanding is similar to DSAdmin ? (except specific interface to create/open).

    About the shift of parameters to dictionary I think both approaches
    are valid.
    ... I used a dictionary to pass
    the properties (rather than functions arguments) mainly because some
    resource may have many properties, also because all the
    set-<resource>-prop function have a similar interface.

I understand your choice in the view of the CLI. I just think it shouldn't 
influence the layout of a class which extends SimpleLDAPObject (see §3 of the 
previous statement).

Is your concern (parameters vs dictionary) specific to DirSrv/DSAdmin class ? Have you the same concern for others classes (Replica, Backend, MT, Agreement..) ?

Here's my rationale of why I stand for the parameter-based (choice made after 
a discussion on #python):
	1- parameter-based approach is self-documenting;
	2- interactive python self-completes parameters, so you can use it
		without knowing the whole interface;
	3- a parameter-based function can take a **dict input, converse is not 
true;	
	4- parameter-based saves you from managing default values;
	5- parameter-based makes clear which are the default values;

I agree with all your points. Now the status is not that bad with dictionary when you are using an IDE.
It shows the header comment of the function you keyed and that header can contain the args list and explanation.

Note that an benefit of dictionaries (to provide the properties/attribute) is that it standardizes the interface for all classes. Any create/set-prop methods of any class takes a set of of properties through a dictionary.
With args the interface will vary a lot from let's say MT that have few attributes to RA/DirSrv that have a lot of attributes.

About the default value, yes parameters brings an advantage. Now this is an advantage for lib389 implementation not for lib389 user.

The parameter approach can easily be wrapped elsewhere:
class DSAdmin(..):
    def __init__(host='localhost', port=389, furtherargs={}):
         # doit

class DirSrv():
    def allocate(allargs):
        # put the following code in an utility function
        proxy_params = DSAdmin.__init__.func_code.co_varnames
        params = {k:v  for k,v in allargs.items() if k in proxy_parameters}
        params['furtherargs'] =  {k:v  for k,v in allargs.items() if k not in 
proxy_parameters}
        self.instance = DSAdmin(**params)

Yes 'allocate' is then similar to 'NewAndConnect' I said before.

Let me know.
Regards
thierry

Moreover the utility function could even be used for mapping the human-
readable values to the internal ones.

Let me know + Peace,
R.

Just let me explain why I choose dictionary, it comes
    from what could be future CLI.
    for example we may have a CLI pattern like:

    <admin_cmd> <verb-resource> [parameters] [options]

    admin_cmd::= "dsadmin"
    verb-resource::= <verb-suffix> | <verb-replication> | <verb-backend>

    |...

    <verb-suffix> ::= list-suffix | create-suffix | delete-suffix...
    parameter::= <parameters to connect to the DS instance (port,
    credential..)>
    options::= <option to process the command and also some properties>

    Then we have specific <verb-resource> to define the resource
    properties (entry cache size, directory path, type of replica...)

    They are set-suffix-prop, set-backend-prop...
    Basically once the resource has been created (with some properties),
    any 'properties' of the resource can be read/write by
    set/get-<resource>-prop.

    So for the CLI (or other admin application) we need to send
    prop:value to an instance. prop are then available in an include
    file, but in a human-readable way (like 'suffix') rather than in an
    attribute way (like 'nsds5replicaroot')

    I do not know if I answer correctly your concerns. Just let me know
    if I missed your questions.

    regards
    thierry

On 01/03/2014 01:10 PM, Roberto Polli wrote:

Hi Everybody!

First of all Happy New Year, hope it will be filled of fun!

I am really sorry for not supporting you for so long, but as I told you I
can't still spend too much extra-work time coding :(.

I saw the evolution of the library and I'm glad of the new functionalities
and the split-up of the brooker in various files.

On the other hand I think that the new interface changes don't go on the
simplicity/usability path, so I'd like to understand your targets and give
some suggestions.

On Thursday 12 December 2013 21:58:12 thierry bordaz wrote:

Ticket https://fedorahosted.org/389/ticket/47625 changes the interface
of DirSrv.
This ticket is the porting side of the test cases to the new interface

1- The new interface initialization is substantially different from a
standard> 
client-server interface. A user expects to instantiate objects like this:
  # client =  Telnet(host, port, credential)

2- This is quite the behavior of the LDAPObject superclass, which I would
like to retain so that we can use fakeldaps for unittest

3-  The standard DirSrv.__init__ (and the same is valid for other
methods),
containing a set of parameters is intrinsically documented. Shifting core

parameters in dictionaries:
  a- de-documents parameters and default values outside the method
  signature;
  b- requires parsing and setting of default values;
  Python allows to retain the dict-style stuff using the **magic, which I
  would> 
embrace in this case.

The new Interface Layer would be easily implemented in a subclass.

Let me know + Peace (and sorry again for my absence),
R.

https://fedorahosted.org/389/attachment/ticket/47628/0001-Ticket-47628-po
rt-> testcases-to-new-DirSrv-interface.patch