3:29 a.m.
On 01/08/2014 12:54 PM, Roberto Polli wrote:
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
Hi Roberto,
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.
ok
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