On 21.03.2016 09:11, Oleg Fayans wrote:
Hi Martin,

On 03/16/2016 03:35 PM, Martin Basti wrote:

On 16.03.2016 15:13, Martin Basti wrote:

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

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

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.
That's done because some tests install client separately and then
deliberately install replica the wrong way to test that the installer
fails in a predicted way. That's why this separate uninstall_client
call. The doc string was corrected.


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

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

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
On the other hand, I do not want cleanup with --pdb option, so maybe it
should just fail

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


The license-related patch is attached too

Patch 0029 pushed to:
master: c2042900382190b1c9d7a44bd719cacd804749b3
ipa-4-3: 1d5b8b8781e5d6300c5029bdd68c6ddf98f6ecd3


Patch 27 is on review

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