[389-devel] Please review (tests) 47628: port test cases to new DirSrv interface

Roberto Polli rpolli at babel.it
Wed Jan 8 11:54:32 UTC 2014


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 

>     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)

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.

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.



>     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).

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;

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)


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

-- 
Roberto Polli
Community Manager
Babel S.r.l. - http://www.babel.it - una business unit di Par-Tec S.p.A.
T: +39.06.9826.9651 M: +39.340.652.2736 F: +39.06.9826.9680
P.zza S.Benedetto da Norcia, 33 - 00040 Pomezia (Roma)

CONFIDENZIALE: Questo messaggio ed i suoi allegati sono di carattere 
confidenziale per i destinatari in indirizzo.
E' vietato l'inoltro non autorizzato a destinatari diversi da quelli indicati 
nel messaggio originale.
Se ricevuto per errore, l'uso del contenuto e' proibito; si prega di 
comunicarlo al mittente e cancellarlo immediatamente.


More information about the 389-devel mailing list