Re: [389-devel] 389-ds-base: /bin/sh scripts should use . instead of source
On Friday 24 January 2014 10:02:02 Rob Crittenden wrote: Roberto Polli wrote: On Friday 24 January 2014 07:28:51 Rich Megginson wrote: https://fedorahosted.org/389/ticket/47511 This is just the tracking ticket for the work. Ok. Hope won't take too much (for now I'm fine as I've fixed my local installation :P) I will eventually trace the issue in lib389 README. Thx + Peace, R. -- Roberto Polli Community Manager Babel - a business unit of Par-Tec S.p.A. - http://www.babel.it 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. -- 389-devel mailing list 389-de...@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/389-devel
Re: [389-devel] 389-ds-base: /bin/sh scripts should use . instead of source
On Friday 24 January 2014 07:28:51 Rich Megginson wrote: https://fedorahosted.org/389/ticket/47511 sorry for the double posting :( I just cloned the master repository and it's not fixed. Peace, R. -- Roberto Polli Community Manager Babel - a business unit of Par-Tec S.p.A. - http://www.babel.it 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. -- 389-devel mailing list 389-devel@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/389-devel
Re: [389-devel] Running lib389 tests
On Friday 24 January 2014 15:56:10 thierry bordaz wrote: Do we really need to get rid of __main__ when using nose/py.test ? Obviously you can take it. I would just avoid noise in testing. Moreover they could leave your environment dirty: eg. if a test fails it doesn't run the teardown function. I do not understand how you can test 'list' if you have not 'create' mapping_tree before. I would have created the list_test requirements in the setup phase. (in the link below I created two different backends for two different test cases) https://github.com/ioggstream/dsadmin/blob/merge_lib389/tests/replica_test.py#L54 While I started with dependent tests on dsadmin, I spent some time to change that approach because small changes caused whole testsuites to fail and it was difficult to find where the error was (in the setup? in the test first code? in the second test code? in the library code? ) Peace, R. -- Roberto Polli Community Manager Babel - a business unit of Par-Tec S.p.A. - http://www.babel.it 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. -- 389-devel mailing list 389-devel@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/389-devel
[389-devel] Running lib389 tests
Hi Thierry + @all,
I'd like to play with the new lib389 and try to split DirSrv in two layers:
- the old approach DSAdmin for TCP communication
- DirSrv implementing your interface
essentially I would put
class DirSrv(DSAdmin):
# ...new stuff go here ...
class DSAdmin(SimpleLDAPObject):
# TCP stuff goes here
Can you please tell me:
1- how to run tests
2- which tests should work
3- which is the test environment.
Thx+Peace,
R.
On Thursday 09 January 2014 10:29:10 thierry bordaz wrote:
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
Re: [389-devel] Please review (tests) 47628: port test cases to new DirSrv interface
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-port- 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. -- 389-devel mailing list 389-devel@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/389-devel
Re: [389-devel] Please review CI tests: fix for test case 47490
Sorry T, but I'm still not fine. Hope to give you some feedback in week. Peace, R. -- Roberto Polli Community Manager Babel S.r.l. - http://www.babel.it 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. -- 389-devel mailing list 389-devel@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/389-devel
Re: [389-devel] Please review ticket 47590 (take #2): add/split functions around replication
On Monday 18 November 2013 11:08:01 thierry bordaz wrote: Hi, this is a second review to take into account the recommendation of the first review. Major changes are: * Create a replicaagreement class in brooker * mv init/status/schedule/create in that new class * mv createDefaultReplMgr into the brooker replica class * Handling of error condition with exceptions https://fedorahosted.org/389/attachment/ticket/47590/0002-Ticket-47590-CI-te sts-add-split-functions-around-rep.patch 1- Here and elsewhere: if not suffix: # This is a mandatory parameter of the command... it fails log.warning(createAgreement: suffix is missing) - return None + raise ValueError(suffix is a required value) 2- The method enableReplication should probably go into the Replica class, and be called like conn.replica.enable() 3- enableReplication and agreementInit should raise in case of errors, not return 1 or -1. Managing error codes is extra work: and moreover which value is fine? Greater than zero? Zero? True? 4- If you put replica_createReplMgr in the Replica brooker, just name it create_manager() so that we can call it conn.replica.create_manager(). The replica is inferred from the context! 5- If you move the agreement stuff outside Replica, I would just name the class Agreement and set self.agreement = Agreement(self) or self.replica_agreement -- Roberto Polli Community Manager Babel S.r.l. - http://www.babel.it 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. -- 389-devel mailing list 389-devel@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/389-devel
Re: [389-devel] Please review bug 47586: CI tests: test case for 47490
On Friday 15 November 2013 15:18:26 thierry bordaz wrote: https://fedorahosted.org/389/attachment/ticket/47586/0001-Ticket-47586-CI-te sts-test-case-for-47490.patch 1- I'd replace the following: -from lib389._constants import DN_DM +from lib389 import DN_DM Files starting with _ are private and are just facilities for the developer. We could consider to rename _constants to constants 2- To isolate testing tools from the library I would put args_standalone co in a proper bugfix harness file/class that we can add to lib389, eg: from lib389.bugfix import args_standalone Otherwise people/projects using lib389 will be full of our testing code. Peace, R. -- Roberto Polli Community Manager Babel S.r.l. - http://www.babel.it 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. -- 389-devel mailing list 389-de...@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/389-devel
Re: [389-devel] Please review ticket 47590: add/split functions around replication
Hi T, sorry for the laconicism but that's the only way to contribute today. On Thursday 14 November 2013 22:23:58 thierry bordaz wrote: I agree that 'setup' is commonly used to prepare/initialize a functionality. Now I would prefer verbs of action/unaction like create/delete, enable/disable, set/get/list. With 'setup' verb we may create entries, enable functionality, set properties. If we have a function setupAgreement (that creates the RA and enables it by default) what is the name for the function that delete the agreement 'deleteAgreement' ? As of now we can still use these names, but the essence is: put them in the Tools part and we'll find the right way to use them. So far DSAdmintools mainly contains offline functions (like start/stop instance). I agree we can put setup functions in it. But I wonder if it would be interesting to keep all offline functions in a separated file. Thanks to inheritance, merging two class is free, splitting not. If tomorrow we decide to merge Tools and Admin we just have to mix-in DsAdminTool. http://en.wikipedia.org/wiki/Mixin#In_Python 2- methods like _createDefaultReplMgr are not expected to be used in production, so should be placed in the DSAdminTools section I am not sure. If someone wants to rapidly deploy a replication topology, he would be interested to have a default replication manager. The idea I put under dsadmin is to remove everything that is not essential: less is more (so we have even less base code to test). People should have: * as few methods as possibile; * methods should be explicative and deterministic; * should be able to remeber them; * should be able to find them using self-completion; One issue we had with the old Rich library is that it put all the methods at the root of the class, so whatever you were trying to do you had too many choiches. Moreover the rationale of the behavior was unclear because methods tried to solve automatically as many errors or cases they could... to be clear they were too intelligent for the unacquainted user. In that case we may offer 'createDefaultReplMgr' (without heading '_'). In your opinion what kind of functions would go into DSAdminTools ? all 'setupxxx' functions ? I would put into DSAdminTools whatever method is not enough general to be used in a standard use case. Moreover consider that complex methods - if exposed - should be tested with almost all the input/output. Making complex setup methods makes it quite impossible. Eg. it is fine to decide a default replica type, not fine to put user credentials. To clarify I'm even against using a default database file name for backends. 3- the brooker naming convention is based on the function-first so that python interactive users can tab and autocomplete it. initAgreement should be renamed to something like agreement_init or something else. For the return codes, simply use exceptions. ok. So do you prefer names like 'replica_create', 'suffix_create', 'agreement_create', rather than 'createReplica', 'createSuffix', 'createAgreement' ? Ok I will change the name. Right. If you started using ipython and its self-completion stuff you'll easily understand the gain of that approach! It may be worth checking the differences between the original dsadmin code used for bugfix and the latest. Sorry again for the short time I had to write you this mail :( Peace, R. = exception handling None return = 1- in case of errors, a method should raise a proper exception and eventually log the error 2- so the assert clauses should be replaced by exception because they mean that something went wrong and an action should be taken 3- about the bindmethod stuff: see http://pastebin.com/w0WnVQuJ Absolutely, I will change the error handling. In addition, exception makes most of the time the code easier to read. Thanks I will resend a review according to you suggestions. regards thierry There are some other points but the best way to set them is with patches. Let me know + Peace, R. On Wednesday 13 November 2013 17:06:25 thierry bordaz wrote: In order to implement the first CI test with replication instances, I reorganised lib389 functions related to replication setup. https://fedorahosted.org/389/attachment/ticket/47590/0001-Ticket-47590-CI -te sts-add-split-functions-around-rep.patch -- Roberto Polli Community Manager Babel S.r.l. - http://www.babel.it 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. -- 389-devel mailing list 389-devel
Re: [389-devel] Please review ticket 47590: add/split functions around replication
Hi Thierry, my consideration follows (a github-like platform with inline comments would be really welcome)! = method naming and placement = 1- I would use the following convention: if a method setup a functionality adding various entries to the tree, I would name it setup and hopefully should be placed DSAdminTools. 2- methods like _createDefaultReplMgr are not expected to be used in production, so should be placed in the DSAdminTools section 3- the brooker naming convention is based on the function-first so that python interactive users can tab and autocomplete it. initAgreement should be renamed to something like agreement_init or something else. For the return codes, simply use exceptions. = exception handling None return = 1- in case of errors, a method should raise a proper exception and eventually log the error 2- so the assert clauses should be replaced by exception because they mean that something went wrong and an action should be taken 3- about the bindmethod stuff: see http://pastebin.com/w0WnVQuJ There are some other points but the best way to set them is with patches. Let me know + Peace, R. On Wednesday 13 November 2013 17:06:25 thierry bordaz wrote: In order to implement the first CI test with replication instances, I reorganised lib389 functions related to replication setup. https://fedorahosted.org/389/attachment/ticket/47590/0001-Ticket-47590-CI-te sts-add-split-functions-around-rep.patch -- Roberto Polli Community Manager Babel S.r.l. - http://www.babel.it 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. -- 389-devel mailing list 389-devel@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/389-devel
Re: [389-devel] Please review lib389 ticket 47584: CI tests: add backup/restore of an instance
On Wednesday 06 November 2013 14:26:22 thierry bordaz wrote: for example log.level=info mean we log fatal/warning/debug ? Yes. Check import logging documentation. the verbose+log approach may have some performance advantage in webapp, but I think that's not the case see - http://dound.com/2010/02/python-logging-performance/ - http://stackoverflow.com/questions/4148790/lazy-logger-message-string-evaluation Peace, R: -- Roberto Polli Community Manager Babel S.r.l. - http://www.babel.it 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. -- 389-devel mailing list 389-de...@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/389-devel
Re: [389-devel] Please review lib389 ticket 47584 (take #2): CI tests: add backup/restore of an instance
You may consider replacing the whole function checkInstanceBackupFS with the simpler: backup_pattern = os.path.join(backupDir, backup*.tar.gz) return glob.glob(backup_pattern) It should return an empty list in case of unexistent path On Wednesday 06 November 2013 18:31:40 thierry bordaz wrote: Thanks Roberto and Rich for reviewing this. This second patch takes into account your recommendation to let the logging mechanism to choose rather that to use 'verbose' flag. https://fedorahosted.org/389/attachment/ticket/47584/0002-Ticket-47584-CI-te sts-add-backup-restore-of-an-insta.patch -- Roberto Polli Community Manager Babel S.r.l. - http://www.babel.it 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. -- 389-devel mailing list 389-de...@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/389-devel
Re: [389-devel] Please review lib389 ticket 47584: CI tests: add backup/restore of an instance
On Wednesday 06 November 2013 14:12:33 thierry bordaz wrote: https://fedorahosted.org/389/attachment/ticket/47584/0001-Ticket-47584-CI-te sts-add-backup-restore-of-an-insta.patch I would delegate all the verbose stuff to log level, so just use log.debug() instead of if verbose: log.debug() Peace, R. -- Roberto Polli Community Manager Babel S.r.l. - http://www.babel.it 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. -- 389-devel mailing list 389-devel@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/389-devel
[389-devel] lib389: cleanup __init__
Hi @all, I started investigating in mocking with fakeldap, and it seems an easy and viable way of adding unittests. A main issue is the DSAdmin.__init__ complexity. I thought - a long time ago actually - to remove from DSAdmin all cached references to backends, suffixes and configuration. If we want to add a cache layer we can do it afterward. And with some cache pattern. Let me know + Peace, R. -- Roberto Polli Community Manager Babel S.r.l. - http://www.babel.it 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. -- 389-devel mailing list 389-de...@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/389-devel
Re: [389-devel] lib389: cleanup __init__
Hi Rich, On Thursday 31 October 2013 10:32:13 Rich Megginson wrote: I thought - a long time ago actually - to remove from DSAdmin all cached references to backends, suffixes and configuration. Part of the complexity is due to trying to keep data across a restart I agree with credential caching - as they should be quite unmutable, and I was talking about __initPart2() which is called by __init__. Are all the __initPart2() attributes essential? Peace, R: -- Roberto Polli Community Manager Babel S.r.l. - http://www.babel.it 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. -- 389-devel mailing list 389-de...@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/389-devel
Re: [389-devel] lib389: cleanup __init__
On Thursday 31 October 2013 10:40:25 Rich Megginson wrote: Are all the __initPart2() attributes essential? No. You could do lazy evaluation of those fields. For example, instead of having a .dbdir field, have a .getdbdir() member that would do an ldapsearch if .dbdir is None. That's good. We could put that in Config. Peace, R: -- Roberto Polli Community Manager Babel S.r.l. - http://www.babel.it 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. -- 389-devel mailing list 389-de...@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/389-devel
Re: [389-devel] Please review lib389 ticket 47575: add test case for ticket47560
All imho. That kind of CI testing is for 389 so feel free to implement it how you prefer. from lib389._constants import DN_DM _constants is private. Import directly from lib389 ;) For testing purposes, we could create harnesses for setup/remove new instances. For test names: m1c1_test.py # a novice won't understand Peace, R On Wednesday 30 October 2013 17:47:44 thierry bordaz wrote: Hello, This tickets implement a test case and propose a layout of the CI tests in the 389-ds. The basic idea is to put CI tests under: head/dirsrvtests/ tickets/ standalone_test.py m1c1_test.py m2_c1_test.py ... testsuites/ acl_test.py replication_test.py ... For example, test_standalone.py would setup a standalone topology and will contain all ticket test cases that are applicable on standalone topology. https://fedorahosted.org/389/attachment/ticket/47575/0001-Ticket-47575-CI-te st-add-test-case-for-ticket47560.patch regards thierry -- Roberto Polli Community Manager Babel S.r.l. - http://www.babel.it 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. -- 389-devel mailing list 389-devel@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/389-devel
Re: [389-devel] Proof of concept: mocking DS in lib389
Hi @all, Jan wrote: I am in fact creating MockDS class with custom ldapadd, actually I agree with Jan about how the mocking process works: 1- reuse/create an ldap mocking class; 2- rewire existing tests on the mock; Hence, a unit test. Ok, I agree to add unit-testing. Just I don't want to move integration tests (the one against a real instance) in another repo. I'd leave the integration tests there so that whenever somebody clones he can test if the lib works for his installation. It will *not* verify the correctness of ldapadd method of real DSInstance class (that is the job of ldapadd`s unit test About testing ldapadd co, we should consider that - as of now - lib389 wraps some python-ldap methods with *args, **kwds. We should even be more consistent in parameter names between wrapping and wrapped method . Peace, R. On Tuesday 29 October 2013 14:18:59 Jan Rusnacko wrote: Hello Thierry, I am not rewriting ldapadd,... methods of real DS class, I am in fact creating MockDS class with custom ldapadd,... methods, _just_ like you suggest :) Furthermore, you can view it as a subclass of real_ds - even though it is not a proper Python subclass, it inherits all functions from repl module just like real_ds would (again through ModuleProxy mechanism). So, methods that are defined in repl are the same for real_ds class and for MockDS class, but ldap.. methods are different. So, basically exactly what you suggest :) Code of the whole class along with all methods is in file tests/test_dsmodules/conftest.py line 7. Thank you, Jan On 10/28/2013 12:02 PM, thierry bordaz wrote: Hi Jan, That is very impressive POC, far above my skill in python. Thanks for sharing this. I have a novice question. This implementation overwrites the basic ldapadd,ldapsearch... function of the real DS. An other approach is to write a 'mock_ds' class being a subclass of 'real_ds' and to overwrite the ldapadd,ldapsearch in mock_ds class (to store data into a dict). What would be the advantages of your approach ? best regards thierry On 10/25/2013 09:36 PM, Jan Rusnacko wrote: Hello Roberto and Thierry, as I promised, I am sending you a proof-of-concept code that demonstrates, how we can mock DS in unit tests for library function (see attachment). You can run tests just by executing py.test in tests directory. Only 3 files are of interest here: lib389/dsmodules/repl.py - this is a Python module with functions - they expect DS instance as the first argument. Since they are functions, not methods, I can just mock DS and pass that fake one as the first argument to them in unit tests. tests/test_dsmodules/conftest.py - this file contains definition of mock DS class along with py.test fixture, that returns it. tests/test_dsmodules/test_repl.py - this contains unit tests for functions from repl.py. What I do is quite simple - I override ldapadd, ldapdelete .. methods of mock DS class, so that instead of sending command to real DS instance, they just store the data in 'dit' dictionary (which represents content stored in DS). This way, I can check that when I call e.g. function enable_changelog(..), in the end DS will have correct changelog entry. To put it very bluntly - enable_changelog(..) function just adds correct changelog entry to whatever is passed to it as the first argument. In unit tests, it is mock DS, otherwise it would be real DS class that sends real ldap commands to real DS instance behind. Now I can successfully test that enable_changelog really works, without going into trouble defining DSInstance or ldap calls at all. Also, I believe this approach would work for 95% of all functions in lib389. Another benefit is that unit tests are much faster, than on real DS instance. Sidenote: even though everything is defined in separate namespace of 'repl' module as function, in runtime they can be used as normal methods of class DSInstance. That is handled by DSModuleProxy. We already went through this, but not with Roberto. Hopefully, now with some code in our hands, we will be able to understand each other on this 'mocking' issue and come to conclusions more quickly. Let me know what you think. Thank you, Jan -- Roberto Polli Community Manager Babel S.r.l. - http://www.babel.it 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. -- 389-devel mailing list 389-devel@lists.fedoraproject.org https
Re: [389-devel] Proof of concept: mocking DS in lib389
Hi @all, Jan wrote: as I promised, I am sending you a proof-of-concept code that demonstrates, how we can mock DS in unit tests for library function Ok, that's clear. instead of sending command to real DS instance, they just store the data in 'dit' dictionary We could use some monkeypatching lib. https://pypi.python.org/pypi/fakeldap/0.5.1 Now I can successfully test that enable_changelog really works, _really_ is not the right word, right : this approach would work for 95% of all functions in lib389. I agree that many functions are just ADD/DELETE, and this will work for that. Mocking functions involving MOD, default values, simulating errors co will be complex though: that's the reason why - even after adding unit tests - I will leave the integration testings in the same repo. DSInstance. That is handled by DSModuleProxy. We already went through this, but not with Roberto. Saw the code, it just imports all files in the folder, right? It's a nice trick, even if it makes the design a bit complex. Peace, R. -- Roberto Polli Community Manager Babel S.r.l. - http://www.babel.it 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. -- 389-devel mailing list 389-devel@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/389-devel
Re: [389-devel] Please review lib389 ticket 47568: Rename DSAdmin class (2nd)
My comments (a github like platform for comment could be really useful) https://fedorahosted.org/389/attachment/ticket/47568/0002-Ticket-47568-Renam e-DSAdmin-class.patch line: comment lib389/__init__.py:1: the module is lib389, not dirsrv lib389/brooker.py:795: python variable naming convention: I would get stick with the _ instead of camelCase and change whenever possible. tests/dsadmin_test.py: I renamed it lib389_test.py, you can merge my changes tests/dsadmin_test.py:39: why remove the addbackend_harn? tests/replica_test.py:119: you're using Backend.delete in a class that should test just Replica. I would use harness and the standard python-ldap methods in setup/teardown, so that we can change the Backend and Replica class without at least breaking the tests. Let me know + Peace, R. -- Roberto Polli Community Manager Babel S.r.l. - http://www.babel.it 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. -- 389-devel mailing list 389-de...@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/389-devel
Re: [389-devel] Please review lib389 ticket 47568: Rename DSAdmin class (2nd)
On Friday 25 October 2013 11:18:53 thierry bordaz wrote: lib389/brooker.py:795: python variable naming convention: I would get stick with the _ instead of camelCase and change whenever possible. If you prefer to use '_' also for local variable, I am fine. Using camel just for classes is more explicative, and I find that _ are easier to read and replace with sed ;) tests/dsadmin_test.py: I renamed it lib389_test.py, you can merge my changes tests/dsadmin_test.py:39: why remove the addbackend_harn? Humm, to be honest... I do not know how to rename files :-) git mv dsadmin_test.py lib389_test.py ;) tests/replica_test.py:119: you're using Backend.delete in a class that should test just Replica. I would use harness and the standard python-ldap methods in setup/teardown, so that we can change the Backend and Replica class without at least breaking the tests. I miss your point. It is calling in teardown conn.backend.delete, is that the call that is not correct ? That's just an IMHO: see those cases: 1- I change the Backend class and break the replica test: I'll look for errors in Replica while the issue is in Backend 2- somebody works on the Backend class, I work on the Replica one: he can break my tests. Splitting the test stuff in an harness module will reduce the impact of all that. As an example, I could even agree the setup process be done populating entries via an LDIF. If I test Replica, Backend or Suffix I shouldn't have other dependencies distracting me. Let me know + Peace, R. -- Roberto Polli Community Manager Babel S.r.l. - http://www.babel.it 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. -- 389-devel mailing list 389-devel@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/389-devel
