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