On 16.03.2016 14:59, Oleg Fayans wrote:
Hi Martin

On 03/16/2016 02:39 PM, Martin Basti wrote:

On 16.03.2016 10:59, Oleg Fayans wrote:
With this patch applied integration tests pass and in-tree tests are
gracefully skipped.

@mkubik, It is not possible to put the decorator to util.py as per our
discussion, because it uses tasks, so tasks must be imported. But tasks
already import util, which leads to circular imports. So I've put it to
tasks.py



NACK

1)
Use right ticket in commit message (#5723)
But (#5736) is exactly the issue that is being addressed. Probably note
both tickets in the commit message?
But as I wrote in ticket #5736, this ticket should be closed, because issue is caused by ticket which is not finished yet, so we should continue just with original ticket.


2)
Link to ticket should be last in the commit message

3)
dereplicafy

3a)
wrong doc string, it removes *only* replicas not clients
No, in fact it removes both:
uninstall_replica(args[0].master, host)
uninstall_client(host)

Both tasks have raiseonerr set to False, which means that even if
replica was not installed but the client was - it will also be removed
I see just
for host in args[0].replicas

I don't see any
for host in args[0].clients
there

Also uninstall_client should not be there. ipa-server-install --uninstall removes client too. The extra call of uninstall client is IMO there just because an ancient bug that is already fixed.


3b)
can we rename it to something different? (replicas_cleanup,
replicas_uninstall, replicas_teardown)
replicas_cleanup, or even topo_cleanup sounds OK to me.

4)
Please fix commit message
- Wile trated correctly
- followiong
- rewrote -> rewrite
Will do

5)
decorator
+    def wrapped(*args):
+        func(*args)
+        for host in args[0].replicas:

Shouldn't be there try-finally around func() call, or something?
No, the wrapped function is a test_* method: if it fails we need to see
the original failure
but if something raise an exception in func(), cleanup will not be executed.

You can do
In [4]: try:
   ...:     raise ValueError('Hello')
   ...: finally:
   ...:     try:
   ...:         raise ValueError('Cleanup')
   ...:     except Exception:
   ...:         pass
   ...:
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-4-affb927f7603> in <module>()
      1 try:
----> 2     raise ValueError('Hello')
      3 finally:
      4     try:
      5         raise ValueError('Cleanup')

ValueError: Hello


Are you sure that there is no need to return result of func()?
The same applies here: we never return results from test_* methods
ok

*) Please create additional patch that will add licence there


Will do :)



--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to