Re: [Freeipa-devel] [PATCH] First part of integration tests for Topology Plugin

2015-08-28 Thread Martin Basti



On 08/27/2015 07:21 PM, Oleg Fayans wrote:

Hi Martin,

My bad, forgot to do git add.


On 08/27/2015 06:27 PM, Martin Basti wrote:



On 08/27/2015 05:41 PM, Oleg Fayans wrote:

Hi,

I am sorry I have missed that.
Fixed. The test fails now due to this bug:

https://fedorahosted.org/freeipa/ticket/5222

The test output is attached together with the updated patch

On 08/26/2015 05:53 PM, Martin Basti wrote:



On 08/26/2015 05:42 PM, Martin Basti wrote:



On 08/26/2015 02:53 PM, Oleg Fayans wrote:

Hi,

No more short links :)

On 08/26/2015 11:50 AM, Tomas Babej wrote:



On 08/26/2015 11:44 AM, Oleg Fayans wrote:

Hi Martin,

On 08/20/2015 11:18 AM, Martin Basti wrote:



On 08/20/2015 10:26 AM, Martin Basti wrote:



On 08/19/2015 04:17 PM, Martin Basti wrote:

I got this:

https://paste.fedoraproject.org/256746/43999380/

FYI replica install failure. (I will retest it, but I'm pretty
sure
that it was clean VM, test for some reason install client first)

   File
/usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py, 





line 295, in decorated
 func(installer)
   File
/usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py, 





line 319, in install_check
 sys.exit(IPA client is already configured on this 
system.\n


2015-08-19T14:14:15Z DEBUG The ipa-replica-install command 
failed,

exception: SystemExit: IPA client is already configured on this
system.
Please uninstall it first before configuring the replica, using
'ipa-client-install --uninstall'.
2015-08-19T14:14:15Z ERROR IPA client is already configured on
this
system.
Please uninstall it first before configuring the replica, using
'ipa-client-install --uninstall'.

Confirm I got same error.

Fixed. It was a regex error.





On 08/19/2015 09:00 AM, Oleg Fayans wrote:

Hi Martin,

As discussed, here is a new version with pep8-related fixes


On 08/14/2015 10:44 AM, Oleg Fayans wrote:

Hi Martin,

Already noticed that. Implemented the named groups as Tomas
advised.
Added the third test for
http://www.freeipa.org/page/V4/Manage_replication_topology/Test_plan#Test_case:_Removal_of_a_topology_segment_is_allowed_only_if_there_is_at_least_one_more_segment_connecting_the_given_replica 










On 08/13/2015 05:06 PM, Martin Basti wrote:



On 08/11/2015 03:36 PM, Oleg Fayans wrote:

Hi Martin,

On 08/11/2015 02:02 PM, Martin Basti wrote:

NACK, comments inline.

On 11/08/15 13:25, Oleg Fayans wrote:

Hi Martin,

Thanks for the review!

On 08/10/2015 07:08 PM, Martin Basti wrote:

Thank you for patch, I have a few nitpicks:

1)
On 10/08/15 13:05, Oleg Fayans wrote:

+def create_segment(master, leftnode, rightnode):
+create_segment(master, leftnode, rightnode)

Why do you add the name of method in docstring?

My bad, fixed.


still there

+ tokenize_topologies(command_output)
+takes an output of `ipa topologysegment-find` and
returns an
array of


Fixed, sorry.





2)

+def create_segment(master, leftnode, rightnode):
+create_segment(master, leftnode, rightnode)
+creates a topology segment. The first argument is a
node to
run the
command on
+The first 3 arguments should be objects of class 
Host

+Returns a hash object containing segment's name,
leftnode,
rightnode information
+

I would prefer to add assert there instead of just
document
that a
Host
object is needed
assert(isinstance(master, Host))
...


Fixed. Created a decorator that checks the type of
arguments


This does not scale well.
If we will want to add new argument that is not host
object, you
need
change it again.


Agreed. Modified the decorator so that you can specify a
slice of
arguments to be checked and a class to compare to. This 
does

scale :)


This might be used as function with specified variables 
that

have to be
host objects




3)
+def destroy_segment(master, segment_name):
+
+destroy_segment(master, segment_name)
+Destroys topology segment. First argument should be
object of
class
Host

Instead of description of params as first, second etc.,
you
may use
following:

+def destroy_segment(master, segment_name):
+
+Destroys topology segment.
+:param master: reference to master object of class
Host
+:param segment: name fo segment
and eventually this in other methods
+:returns: Lorem ipsum sit dolor mit amet
+:raises NotFound: if segment is not found


Fixed


4)

cls.replicas[:len(cls.replicas) - 1],

I suggest cls.replicas[:-1]

In [2]: a = [1, 2, 3, 4, 5]

In [3]: a[:-1]
Out[3]: [1, 2, 3, 4]


Fixed


5)
Why re.findall() and then you just use the first result?
'leftnode': self.leftnode_re.findall(i)[0]

Isn't just re.search() enough?
leftnode_re.search(value).group(1)


in fact
leftnode_re.findall(string)[0]
and
leftnode_re.search(string).group(1),
Are equally bad from the readability point of view. The
first
one is
even shorter a bit, so why change? :)


It depends on point of view, because when I reviewed it
yesterday my
brain raises exception that 

Re: [Freeipa-devel] [PATCH] First part of integration tests for Topology Plugin

2015-08-27 Thread Oleg Fayans

Hi,

I am sorry I have missed that.
Fixed. The test fails now due to this bug:

https://fedorahosted.org/freeipa/ticket/5222

The test output is attached together with the updated patch

On 08/26/2015 05:53 PM, Martin Basti wrote:



On 08/26/2015 05:42 PM, Martin Basti wrote:



On 08/26/2015 02:53 PM, Oleg Fayans wrote:

Hi,

No more short links :)

On 08/26/2015 11:50 AM, Tomas Babej wrote:



On 08/26/2015 11:44 AM, Oleg Fayans wrote:

Hi Martin,

On 08/20/2015 11:18 AM, Martin Basti wrote:



On 08/20/2015 10:26 AM, Martin Basti wrote:



On 08/19/2015 04:17 PM, Martin Basti wrote:

I got this:

https://paste.fedoraproject.org/256746/43999380/

FYI replica install failure. (I will retest it, but I'm pretty sure
that it was clean VM, test for some reason install client first)

   File
/usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py,


line 295, in decorated
 func(installer)
   File
/usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py,


line 319, in install_check
 sys.exit(IPA client is already configured on this system.\n

2015-08-19T14:14:15Z DEBUG The ipa-replica-install command failed,
exception: SystemExit: IPA client is already configured on this
system.
Please uninstall it first before configuring the replica, using
'ipa-client-install --uninstall'.
2015-08-19T14:14:15Z ERROR IPA client is already configured on this
system.
Please uninstall it first before configuring the replica, using
'ipa-client-install --uninstall'.

Confirm I got same error.

Fixed. It was a regex error.





On 08/19/2015 09:00 AM, Oleg Fayans wrote:

Hi Martin,

As discussed, here is a new version with pep8-related fixes


On 08/14/2015 10:44 AM, Oleg Fayans wrote:

Hi Martin,

Already noticed that. Implemented the named groups as Tomas
advised.
Added the third test for
http://www.freeipa.org/page/V4/Manage_replication_topology/Test_plan#Test_case:_Removal_of_a_topology_segment_is_allowed_only_if_there_is_at_least_one_more_segment_connecting_the_given_replica







On 08/13/2015 05:06 PM, Martin Basti wrote:



On 08/11/2015 03:36 PM, Oleg Fayans wrote:

Hi Martin,

On 08/11/2015 02:02 PM, Martin Basti wrote:

NACK, comments inline.

On 11/08/15 13:25, Oleg Fayans wrote:

Hi Martin,

Thanks for the review!

On 08/10/2015 07:08 PM, Martin Basti wrote:

Thank you for patch, I have a few nitpicks:

1)
On 10/08/15 13:05, Oleg Fayans wrote:

+def create_segment(master, leftnode, rightnode):
+create_segment(master, leftnode, rightnode)

Why do you add the name of method in docstring?

My bad, fixed.


still there

+tokenize_topologies(command_output)
+takes an output of `ipa topologysegment-find` and
returns an
array of


Fixed, sorry.





2)

+def create_segment(master, leftnode, rightnode):
+create_segment(master, leftnode, rightnode)
+creates a topology segment. The first argument is a
node to
run the
command on
+The first 3 arguments should be objects of class Host
+Returns a hash object containing segment's name,
leftnode,
rightnode information
+

I would prefer to add assert there instead of just document
that a
Host
object is needed
assert(isinstance(master, Host))
...


Fixed. Created a decorator that checks the type of arguments


This does not scale well.
If we will want to add new argument that is not host
object, you
need
change it again.


Agreed. Modified the decorator so that you can specify a
slice of
arguments to be checked and a class to compare to. This does
scale :)


This might be used as function with specified variables that
have to be
host objects




3)
+def destroy_segment(master, segment_name):
+
+destroy_segment(master, segment_name)
+Destroys topology segment. First argument should be
object of
class
Host

Instead of description of params as first, second etc., you
may use
following:

+def destroy_segment(master, segment_name):
+
+Destroys topology segment.
+:param master: reference to master object of class Host
+:param segment: name fo segment
and eventually this in other methods
+:returns: Lorem ipsum sit dolor mit amet
+:raises NotFound: if segment is not found


Fixed


4)

cls.replicas[:len(cls.replicas) - 1],

I suggest cls.replicas[:-1]

In [2]: a = [1, 2, 3, 4, 5]

In [3]: a[:-1]
Out[3]: [1, 2, 3, 4]


Fixed


5)
Why re.findall() and then you just use the first result?
'leftnode': self.leftnode_re.findall(i)[0]

Isn't just re.search() enough?
leftnode_re.search(value).group(1)


in fact
leftnode_re.findall(string)[0]
and
leftnode_re.search(string).group(1),
Are equally bad from the readability point of view. The first
one is
even shorter a bit, so why change? :)


It depends on point of view,  because when I reviewed it
yesterday my
brain raises exception that you are trying to add multiple
values to
single value attribute, because you used findall, I expected
that you
need multiple values, and then I saw that index [0] at the
end,
and I
was pretty 

Re: [Freeipa-devel] [PATCH] First part of integration tests for Topology Plugin

2015-08-27 Thread Oleg Fayans

Hi Martin,

My bad, forgot to do git add.


On 08/27/2015 06:27 PM, Martin Basti wrote:



On 08/27/2015 05:41 PM, Oleg Fayans wrote:

Hi,

I am sorry I have missed that.
Fixed. The test fails now due to this bug:

https://fedorahosted.org/freeipa/ticket/5222

The test output is attached together with the updated patch

On 08/26/2015 05:53 PM, Martin Basti wrote:



On 08/26/2015 05:42 PM, Martin Basti wrote:



On 08/26/2015 02:53 PM, Oleg Fayans wrote:

Hi,

No more short links :)

On 08/26/2015 11:50 AM, Tomas Babej wrote:



On 08/26/2015 11:44 AM, Oleg Fayans wrote:

Hi Martin,

On 08/20/2015 11:18 AM, Martin Basti wrote:



On 08/20/2015 10:26 AM, Martin Basti wrote:



On 08/19/2015 04:17 PM, Martin Basti wrote:

I got this:

https://paste.fedoraproject.org/256746/43999380/

FYI replica install failure. (I will retest it, but I'm pretty
sure
that it was clean VM, test for some reason install client first)

   File
/usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py,



line 295, in decorated
 func(installer)
   File
/usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py,



line 319, in install_check
 sys.exit(IPA client is already configured on this system.\n

2015-08-19T14:14:15Z DEBUG The ipa-replica-install command failed,
exception: SystemExit: IPA client is already configured on this
system.
Please uninstall it first before configuring the replica, using
'ipa-client-install --uninstall'.
2015-08-19T14:14:15Z ERROR IPA client is already configured on
this
system.
Please uninstall it first before configuring the replica, using
'ipa-client-install --uninstall'.

Confirm I got same error.

Fixed. It was a regex error.





On 08/19/2015 09:00 AM, Oleg Fayans wrote:

Hi Martin,

As discussed, here is a new version with pep8-related fixes


On 08/14/2015 10:44 AM, Oleg Fayans wrote:

Hi Martin,

Already noticed that. Implemented the named groups as Tomas
advised.
Added the third test for
http://www.freeipa.org/page/V4/Manage_replication_topology/Test_plan#Test_case:_Removal_of_a_topology_segment_is_allowed_only_if_there_is_at_least_one_more_segment_connecting_the_given_replica








On 08/13/2015 05:06 PM, Martin Basti wrote:



On 08/11/2015 03:36 PM, Oleg Fayans wrote:

Hi Martin,

On 08/11/2015 02:02 PM, Martin Basti wrote:

NACK, comments inline.

On 11/08/15 13:25, Oleg Fayans wrote:

Hi Martin,

Thanks for the review!

On 08/10/2015 07:08 PM, Martin Basti wrote:

Thank you for patch, I have a few nitpicks:

1)
On 10/08/15 13:05, Oleg Fayans wrote:

+def create_segment(master, leftnode, rightnode):
+create_segment(master, leftnode, rightnode)

Why do you add the name of method in docstring?

My bad, fixed.


still there

+ tokenize_topologies(command_output)
+takes an output of `ipa topologysegment-find` and
returns an
array of


Fixed, sorry.





2)

+def create_segment(master, leftnode, rightnode):
+create_segment(master, leftnode, rightnode)
+creates a topology segment. The first argument is a
node to
run the
command on
+The first 3 arguments should be objects of class Host
+Returns a hash object containing segment's name,
leftnode,
rightnode information
+

I would prefer to add assert there instead of just
document
that a
Host
object is needed
assert(isinstance(master, Host))
...


Fixed. Created a decorator that checks the type of
arguments


This does not scale well.
If we will want to add new argument that is not host
object, you
need
change it again.


Agreed. Modified the decorator so that you can specify a
slice of
arguments to be checked and a class to compare to. This does
scale :)


This might be used as function with specified variables that
have to be
host objects




3)
+def destroy_segment(master, segment_name):
+
+destroy_segment(master, segment_name)
+Destroys topology segment. First argument should be
object of
class
Host

Instead of description of params as first, second etc.,
you
may use
following:

+def destroy_segment(master, segment_name):
+
+Destroys topology segment.
+:param master: reference to master object of class
Host
+:param segment: name fo segment
and eventually this in other methods
+:returns: Lorem ipsum sit dolor mit amet
+:raises NotFound: if segment is not found


Fixed


4)

cls.replicas[:len(cls.replicas) - 1],

I suggest cls.replicas[:-1]

In [2]: a = [1, 2, 3, 4, 5]

In [3]: a[:-1]
Out[3]: [1, 2, 3, 4]


Fixed


5)
Why re.findall() and then you just use the first result?
'leftnode': self.leftnode_re.findall(i)[0]

Isn't just re.search() enough?
leftnode_re.search(value).group(1)


in fact
leftnode_re.findall(string)[0]
and
leftnode_re.search(string).group(1),
Are equally bad from the readability point of view. The
first
one is
even shorter a bit, so why change? :)


It depends on point of view,  because when I reviewed it
yesterday my
brain raises exception that you are trying to add multiple
values to
single value attribute, 

Re: [Freeipa-devel] [PATCH] First part of integration tests for Topology Plugin

2015-08-27 Thread Martin Basti



On 08/27/2015 05:41 PM, Oleg Fayans wrote:

Hi,

I am sorry I have missed that.
Fixed. The test fails now due to this bug:

https://fedorahosted.org/freeipa/ticket/5222

The test output is attached together with the updated patch

On 08/26/2015 05:53 PM, Martin Basti wrote:



On 08/26/2015 05:42 PM, Martin Basti wrote:



On 08/26/2015 02:53 PM, Oleg Fayans wrote:

Hi,

No more short links :)

On 08/26/2015 11:50 AM, Tomas Babej wrote:



On 08/26/2015 11:44 AM, Oleg Fayans wrote:

Hi Martin,

On 08/20/2015 11:18 AM, Martin Basti wrote:



On 08/20/2015 10:26 AM, Martin Basti wrote:



On 08/19/2015 04:17 PM, Martin Basti wrote:

I got this:

https://paste.fedoraproject.org/256746/43999380/
FYI replica install failure. (I will retest it, but I'm pretty 
sure

that it was clean VM, test for some reason install client first)

   File
/usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py, 




line 295, in decorated
 func(installer)
   File
/usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py, 




line 319, in install_check
 sys.exit(IPA client is already configured on this system.\n

2015-08-19T14:14:15Z DEBUG The ipa-replica-install command failed,
exception: SystemExit: IPA client is already configured on this
system.
Please uninstall it first before configuring the replica, using
'ipa-client-install --uninstall'.
2015-08-19T14:14:15Z ERROR IPA client is already configured on 
this

system.
Please uninstall it first before configuring the replica, using
'ipa-client-install --uninstall'.

Confirm I got same error.

Fixed. It was a regex error.





On 08/19/2015 09:00 AM, Oleg Fayans wrote:

Hi Martin,

As discussed, here is a new version with pep8-related fixes


On 08/14/2015 10:44 AM, Oleg Fayans wrote:

Hi Martin,

Already noticed that. Implemented the named groups as Tomas
advised.
Added the third test for
http://www.freeipa.org/page/V4/Manage_replication_topology/Test_plan#Test_case:_Removal_of_a_topology_segment_is_allowed_only_if_there_is_at_least_one_more_segment_connecting_the_given_replica 









On 08/13/2015 05:06 PM, Martin Basti wrote:



On 08/11/2015 03:36 PM, Oleg Fayans wrote:

Hi Martin,

On 08/11/2015 02:02 PM, Martin Basti wrote:

NACK, comments inline.

On 11/08/15 13:25, Oleg Fayans wrote:

Hi Martin,

Thanks for the review!

On 08/10/2015 07:08 PM, Martin Basti wrote:

Thank you for patch, I have a few nitpicks:

1)
On 10/08/15 13:05, Oleg Fayans wrote:

+def create_segment(master, leftnode, rightnode):
+create_segment(master, leftnode, rightnode)

Why do you add the name of method in docstring?

My bad, fixed.


still there

+ tokenize_topologies(command_output)
+takes an output of `ipa topologysegment-find` and
returns an
array of


Fixed, sorry.





2)

+def create_segment(master, leftnode, rightnode):
+create_segment(master, leftnode, rightnode)
+creates a topology segment. The first argument is a
node to
run the
command on
+The first 3 arguments should be objects of class Host
+Returns a hash object containing segment's name,
leftnode,
rightnode information
+

I would prefer to add assert there instead of just 
document

that a
Host
object is needed
assert(isinstance(master, Host))
...


Fixed. Created a decorator that checks the type of 
arguments


This does not scale well.
If we will want to add new argument that is not host
object, you
need
change it again.


Agreed. Modified the decorator so that you can specify a
slice of
arguments to be checked and a class to compare to. This does
scale :)


This might be used as function with specified variables that
have to be
host objects




3)
+def destroy_segment(master, segment_name):
+
+destroy_segment(master, segment_name)
+Destroys topology segment. First argument should be
object of
class
Host

Instead of description of params as first, second etc., 
you

may use
following:

+def destroy_segment(master, segment_name):
+
+Destroys topology segment.
+:param master: reference to master object of class 
Host

+:param segment: name fo segment
and eventually this in other methods
+:returns: Lorem ipsum sit dolor mit amet
+:raises NotFound: if segment is not found


Fixed


4)

cls.replicas[:len(cls.replicas) - 1],

I suggest cls.replicas[:-1]

In [2]: a = [1, 2, 3, 4, 5]

In [3]: a[:-1]
Out[3]: [1, 2, 3, 4]


Fixed


5)
Why re.findall() and then you just use the first result?
'leftnode': self.leftnode_re.findall(i)[0]

Isn't just re.search() enough?
leftnode_re.search(value).group(1)


in fact
leftnode_re.findall(string)[0]
and
leftnode_re.search(string).group(1),
Are equally bad from the readability point of view. The 
first

one is
even shorter a bit, so why change? :)


It depends on point of view,  because when I reviewed it
yesterday my
brain raises exception that you are trying to add multiple
values to
single value attribute, because you used findall, I expected
that you
need multiple values, and 

Re: [Freeipa-devel] [PATCH] First part of integration tests for Topology Plugin

2015-08-26 Thread Martin Basti



On 08/26/2015 05:42 PM, Martin Basti wrote:



On 08/26/2015 02:53 PM, Oleg Fayans wrote:

Hi,

No more short links :)

On 08/26/2015 11:50 AM, Tomas Babej wrote:



On 08/26/2015 11:44 AM, Oleg Fayans wrote:

Hi Martin,

On 08/20/2015 11:18 AM, Martin Basti wrote:



On 08/20/2015 10:26 AM, Martin Basti wrote:



On 08/19/2015 04:17 PM, Martin Basti wrote:

I got this:

https://paste.fedoraproject.org/256746/43999380/

FYI replica install failure. (I will retest it, but I'm pretty sure
that it was clean VM, test for some reason install client first)

   File
/usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py, 



line 295, in decorated
 func(installer)
   File
/usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py, 



line 319, in install_check
 sys.exit(IPA client is already configured on this system.\n

2015-08-19T14:14:15Z DEBUG The ipa-replica-install command failed,
exception: SystemExit: IPA client is already configured on this 
system.

Please uninstall it first before configuring the replica, using
'ipa-client-install --uninstall'.
2015-08-19T14:14:15Z ERROR IPA client is already configured on this
system.
Please uninstall it first before configuring the replica, using
'ipa-client-install --uninstall'.

Confirm I got same error.

Fixed. It was a regex error.





On 08/19/2015 09:00 AM, Oleg Fayans wrote:

Hi Martin,

As discussed, here is a new version with pep8-related fixes


On 08/14/2015 10:44 AM, Oleg Fayans wrote:

Hi Martin,

Already noticed that. Implemented the named groups as Tomas 
advised.

Added the third test for
http://www.freeipa.org/page/V4/Manage_replication_topology/Test_plan#Test_case:_Removal_of_a_topology_segment_is_allowed_only_if_there_is_at_least_one_more_segment_connecting_the_given_replica 








On 08/13/2015 05:06 PM, Martin Basti wrote:



On 08/11/2015 03:36 PM, Oleg Fayans wrote:

Hi Martin,

On 08/11/2015 02:02 PM, Martin Basti wrote:

NACK, comments inline.

On 11/08/15 13:25, Oleg Fayans wrote:

Hi Martin,

Thanks for the review!

On 08/10/2015 07:08 PM, Martin Basti wrote:

Thank you for patch, I have a few nitpicks:

1)
On 10/08/15 13:05, Oleg Fayans wrote:

+def create_segment(master, leftnode, rightnode):
+create_segment(master, leftnode, rightnode)

Why do you add the name of method in docstring?

My bad, fixed.


still there

+tokenize_topologies(command_output)
+takes an output of `ipa topologysegment-find` and
returns an
array of


Fixed, sorry.





2)

+def create_segment(master, leftnode, rightnode):
+create_segment(master, leftnode, rightnode)
+creates a topology segment. The first argument is a 
node to

run the
command on
+The first 3 arguments should be objects of class Host
+Returns a hash object containing segment's name, 
leftnode,

rightnode information
+

I would prefer to add assert there instead of just document
that a
Host
object is needed
assert(isinstance(master, Host))
...


Fixed. Created a decorator that checks the type of arguments


This does not scale well.
If we will want to add new argument that is not host 
object, you

need
change it again.


Agreed. Modified the decorator so that you can specify a 
slice of

arguments to be checked and a class to compare to. This does
scale :)


This might be used as function with specified variables that
have to be
host objects




3)
+def destroy_segment(master, segment_name):
+
+destroy_segment(master, segment_name)
+Destroys topology segment. First argument should be
object of
class
Host

Instead of description of params as first, second etc., you
may use
following:

+def destroy_segment(master, segment_name):
+
+Destroys topology segment.
+:param master: reference to master object of class Host
+:param segment: name fo segment
and eventually this in other methods
+:returns: Lorem ipsum sit dolor mit amet
+:raises NotFound: if segment is not found


Fixed


4)

cls.replicas[:len(cls.replicas) - 1],

I suggest cls.replicas[:-1]

In [2]: a = [1, 2, 3, 4, 5]

In [3]: a[:-1]
Out[3]: [1, 2, 3, 4]


Fixed


5)
Why re.findall() and then you just use the first result?
'leftnode': self.leftnode_re.findall(i)[0]

Isn't just re.search() enough?
leftnode_re.search(value).group(1)


in fact
leftnode_re.findall(string)[0]
and
leftnode_re.search(string).group(1),
Are equally bad from the readability point of view. The first
one is
even shorter a bit, so why change? :)


It depends on point of view,  because when I reviewed it
yesterday my
brain raises exception that you are trying to add multiple
values to
single value attribute, because you used findall, I expected
that you
need multiple values, and then I saw that index [0] at the 
end,

and I
was pretty confused what are you trying to achieve.

And because findall is not effective in case when you need to
find just
one occurrence.


I got it. Fixed.









Python 3 nitpick:
I'm not sure if time when we should 

Re: [Freeipa-devel] [PATCH] First part of integration tests for Topology Plugin

2015-08-26 Thread Oleg Fayans

Hi Martin,

On 08/20/2015 11:18 AM, Martin Basti wrote:



On 08/20/2015 10:26 AM, Martin Basti wrote:



On 08/19/2015 04:17 PM, Martin Basti wrote:

I got this:

https://paste.fedoraproject.org/256746/43999380/

FYI replica install failure. (I will retest it, but I'm pretty sure
that it was clean VM, test for some reason install client first)

  File
/usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py,
line 295, in decorated
func(installer)
  File
/usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py,
line 319, in install_check
sys.exit(IPA client is already configured on this system.\n

2015-08-19T14:14:15Z DEBUG The ipa-replica-install command failed,
exception: SystemExit: IPA client is already configured on this system.
Please uninstall it first before configuring the replica, using
'ipa-client-install --uninstall'.
2015-08-19T14:14:15Z ERROR IPA client is already configured on this
system.
Please uninstall it first before configuring the replica, using
'ipa-client-install --uninstall'.

Confirm I got same error.

Fixed. It was a regex error.





On 08/19/2015 09:00 AM, Oleg Fayans wrote:

Hi Martin,

As discussed, here is a new version with pep8-related fixes


On 08/14/2015 10:44 AM, Oleg Fayans wrote:

Hi Martin,

Already noticed that. Implemented the named groups as Tomas advised.
Added the third test for
http://www.freeipa.org/page/V4/Manage_replication_topology/Test_plan#Test_case:_Removal_of_a_topology_segment_is_allowed_only_if_there_is_at_least_one_more_segment_connecting_the_given_replica





On 08/13/2015 05:06 PM, Martin Basti wrote:



On 08/11/2015 03:36 PM, Oleg Fayans wrote:

Hi Martin,

On 08/11/2015 02:02 PM, Martin Basti wrote:

NACK, comments inline.

On 11/08/15 13:25, Oleg Fayans wrote:

Hi Martin,

Thanks for the review!

On 08/10/2015 07:08 PM, Martin Basti wrote:

Thank you for patch, I have a few nitpicks:

1)
On 10/08/15 13:05, Oleg Fayans wrote:

+def create_segment(master, leftnode, rightnode):
+create_segment(master, leftnode, rightnode)

Why do you add the name of method in docstring?

My bad, fixed.


still there

+tokenize_topologies(command_output)
+takes an output of `ipa topologysegment-find` and
returns an
array of


Fixed, sorry.





2)

+def create_segment(master, leftnode, rightnode):
+create_segment(master, leftnode, rightnode)
+creates a topology segment. The first argument is a node to
run the
command on
+The first 3 arguments should be objects of class Host
+Returns a hash object containing segment's name, leftnode,
rightnode information
+

I would prefer to add assert there instead of just document
that a
Host
object is needed
assert(isinstance(master, Host))
...


Fixed. Created a decorator that checks the type of arguments


This does not scale well.
If we will want to add new argument that is not host object, you
need
change it again.


Agreed. Modified the decorator so that you can specify a slice of
arguments to be checked and a class to compare to. This does
scale :)


This might be used as function with specified variables that
have to be
host objects




3)
+def destroy_segment(master, segment_name):
+
+destroy_segment(master, segment_name)
+Destroys topology segment. First argument should be
object of
class
Host

Instead of description of params as first, second etc., you
may use
following:

+def destroy_segment(master, segment_name):
+
+Destroys topology segment.
+:param master: reference to master object of class Host
+:param segment: name fo segment
and eventually this in other methods
+:returns: Lorem ipsum sit dolor mit amet
+:raises NotFound: if segment is not found


Fixed


4)

cls.replicas[:len(cls.replicas) - 1],

I suggest cls.replicas[:-1]

In [2]: a = [1, 2, 3, 4, 5]

In [3]: a[:-1]
Out[3]: [1, 2, 3, 4]


Fixed


5)
Why re.findall() and then you just use the first result?
'leftnode': self.leftnode_re.findall(i)[0]

Isn't just re.search() enough?
leftnode_re.search(value).group(1)


in fact
leftnode_re.findall(string)[0]
and
leftnode_re.search(string).group(1),
Are equally bad from the readability point of view. The first
one is
even shorter a bit, so why change? :)


It depends on point of view,  because when I reviewed it
yesterday my
brain raises exception that you are trying to add multiple
values to
single value attribute, because you used findall, I expected
that you
need multiple values, and then I saw that index [0] at the end,
and I
was pretty confused what are you trying to achieve.

And because findall is not effective in case when you need to
find just
one occurrence.


I got it. Fixed.









Python 3 nitpick:
I'm not sure if time when we should enforce python 2/3
compability
already comes, but just for record:
instead of open(file, 'r'), please use io.open(file, 'r')
(import io
before)


Done.








1)

+#

empty comment here (several times)


Removed






NACK

you changed it wrong


Re: [Freeipa-devel] [PATCH] First part of integration tests for Topology Plugin

2015-08-26 Thread Tomas Babej


On 08/26/2015 11:44 AM, Oleg Fayans wrote:
 Hi Martin,
 
 On 08/20/2015 11:18 AM, Martin Basti wrote:


 On 08/20/2015 10:26 AM, Martin Basti wrote:


 On 08/19/2015 04:17 PM, Martin Basti wrote:
 I got this:

 https://paste.fedoraproject.org/256746/43999380/
 FYI replica install failure. (I will retest it, but I'm pretty sure
 that it was clean VM, test for some reason install client first)

   File
 /usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py,

 line 295, in decorated
 func(installer)
   File
 /usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py,

 line 319, in install_check
 sys.exit(IPA client is already configured on this system.\n

 2015-08-19T14:14:15Z DEBUG The ipa-replica-install command failed,
 exception: SystemExit: IPA client is already configured on this system.
 Please uninstall it first before configuring the replica, using
 'ipa-client-install --uninstall'.
 2015-08-19T14:14:15Z ERROR IPA client is already configured on this
 system.
 Please uninstall it first before configuring the replica, using
 'ipa-client-install --uninstall'.
 Confirm I got same error.
 Fixed. It was a regex error.
 


 On 08/19/2015 09:00 AM, Oleg Fayans wrote:
 Hi Martin,

 As discussed, here is a new version with pep8-related fixes


 On 08/14/2015 10:44 AM, Oleg Fayans wrote:
 Hi Martin,

 Already noticed that. Implemented the named groups as Tomas advised.
 Added the third test for
 http://www.freeipa.org/page/V4/Manage_replication_topology/Test_plan#Test_case:_Removal_of_a_topology_segment_is_allowed_only_if_there_is_at_least_one_more_segment_connecting_the_given_replica






 On 08/13/2015 05:06 PM, Martin Basti wrote:


 On 08/11/2015 03:36 PM, Oleg Fayans wrote:
 Hi Martin,

 On 08/11/2015 02:02 PM, Martin Basti wrote:
 NACK, comments inline.

 On 11/08/15 13:25, Oleg Fayans wrote:
 Hi Martin,

 Thanks for the review!

 On 08/10/2015 07:08 PM, Martin Basti wrote:
 Thank you for patch, I have a few nitpicks:

 1)
 On 10/08/15 13:05, Oleg Fayans wrote:
 +def create_segment(master, leftnode, rightnode):
 +create_segment(master, leftnode, rightnode)
 Why do you add the name of method in docstring?
 My bad, fixed.

 still there

 +tokenize_topologies(command_output)
 +takes an output of `ipa topologysegment-find` and
 returns an
 array of

 Fixed, sorry.



 2)

 +def create_segment(master, leftnode, rightnode):
 +create_segment(master, leftnode, rightnode)
 +creates a topology segment. The first argument is a node to
 run the
 command on
 +The first 3 arguments should be objects of class Host
 +Returns a hash object containing segment's name, leftnode,
 rightnode information
 +

 I would prefer to add assert there instead of just document
 that a
 Host
 object is needed
 assert(isinstance(master, Host))
 ...

 Fixed. Created a decorator that checks the type of arguments

 This does not scale well.
 If we will want to add new argument that is not host object, you
 need
 change it again.

 Agreed. Modified the decorator so that you can specify a slice of
 arguments to be checked and a class to compare to. This does
 scale :)

 This might be used as function with specified variables that
 have to be
 host objects


 3)
 +def destroy_segment(master, segment_name):
 +
 +destroy_segment(master, segment_name)
 +Destroys topology segment. First argument should be
 object of
 class
 Host

 Instead of description of params as first, second etc., you
 may use
 following:

 +def destroy_segment(master, segment_name):
 +
 +Destroys topology segment.
 +:param master: reference to master object of class Host
 +:param segment: name fo segment
 and eventually this in other methods
 +:returns: Lorem ipsum sit dolor mit amet
 +:raises NotFound: if segment is not found

 Fixed

 4)

 cls.replicas[:len(cls.replicas) - 1],

 I suggest cls.replicas[:-1]

 In [2]: a = [1, 2, 3, 4, 5]

 In [3]: a[:-1]
 Out[3]: [1, 2, 3, 4]

 Fixed

 5)
 Why re.findall() and then you just use the first result?
 'leftnode': self.leftnode_re.findall(i)[0]

 Isn't just re.search() enough?
 leftnode_re.search(value).group(1)

 in fact
 leftnode_re.findall(string)[0]
 and
 leftnode_re.search(string).group(1),
 Are equally bad from the readability point of view. The first
 one is
 even shorter a bit, so why change? :)

 It depends on point of view,  because when I reviewed it
 yesterday my
 brain raises exception that you are trying to add multiple
 values to
 single value attribute, because you used findall, I expected
 that you
 need multiple values, and then I saw that index [0] at the end,
 and I
 was pretty confused what are you trying to achieve.

 And because findall is not effective in case when you need to
 find just
 one occurrence.

 I got it. Fixed.






 Python 3 nitpick:
 I'm not sure if time when we should enforce python 2/3
 compability
 already comes, but just for record:
 instead of open(file, 'r'), 

Re: [Freeipa-devel] [PATCH] First part of integration tests for Topology Plugin

2015-08-26 Thread Oleg Fayans

Hi,

No more short links :)

On 08/26/2015 11:50 AM, Tomas Babej wrote:



On 08/26/2015 11:44 AM, Oleg Fayans wrote:

Hi Martin,

On 08/20/2015 11:18 AM, Martin Basti wrote:



On 08/20/2015 10:26 AM, Martin Basti wrote:



On 08/19/2015 04:17 PM, Martin Basti wrote:

I got this:

https://paste.fedoraproject.org/256746/43999380/

FYI replica install failure. (I will retest it, but I'm pretty sure
that it was clean VM, test for some reason install client first)

   File
/usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py,

line 295, in decorated
 func(installer)
   File
/usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py,

line 319, in install_check
 sys.exit(IPA client is already configured on this system.\n

2015-08-19T14:14:15Z DEBUG The ipa-replica-install command failed,
exception: SystemExit: IPA client is already configured on this system.
Please uninstall it first before configuring the replica, using
'ipa-client-install --uninstall'.
2015-08-19T14:14:15Z ERROR IPA client is already configured on this
system.
Please uninstall it first before configuring the replica, using
'ipa-client-install --uninstall'.

Confirm I got same error.

Fixed. It was a regex error.





On 08/19/2015 09:00 AM, Oleg Fayans wrote:

Hi Martin,

As discussed, here is a new version with pep8-related fixes


On 08/14/2015 10:44 AM, Oleg Fayans wrote:

Hi Martin,

Already noticed that. Implemented the named groups as Tomas advised.
Added the third test for
http://www.freeipa.org/page/V4/Manage_replication_topology/Test_plan#Test_case:_Removal_of_a_topology_segment_is_allowed_only_if_there_is_at_least_one_more_segment_connecting_the_given_replica






On 08/13/2015 05:06 PM, Martin Basti wrote:



On 08/11/2015 03:36 PM, Oleg Fayans wrote:

Hi Martin,

On 08/11/2015 02:02 PM, Martin Basti wrote:

NACK, comments inline.

On 11/08/15 13:25, Oleg Fayans wrote:

Hi Martin,

Thanks for the review!

On 08/10/2015 07:08 PM, Martin Basti wrote:

Thank you for patch, I have a few nitpicks:

1)
On 10/08/15 13:05, Oleg Fayans wrote:

+def create_segment(master, leftnode, rightnode):
+create_segment(master, leftnode, rightnode)

Why do you add the name of method in docstring?

My bad, fixed.


still there

+tokenize_topologies(command_output)
+takes an output of `ipa topologysegment-find` and
returns an
array of


Fixed, sorry.





2)

+def create_segment(master, leftnode, rightnode):
+create_segment(master, leftnode, rightnode)
+creates a topology segment. The first argument is a node to
run the
command on
+The first 3 arguments should be objects of class Host
+Returns a hash object containing segment's name, leftnode,
rightnode information
+

I would prefer to add assert there instead of just document
that a
Host
object is needed
assert(isinstance(master, Host))
...


Fixed. Created a decorator that checks the type of arguments


This does not scale well.
If we will want to add new argument that is not host object, you
need
change it again.


Agreed. Modified the decorator so that you can specify a slice of
arguments to be checked and a class to compare to. This does
scale :)


This might be used as function with specified variables that
have to be
host objects




3)
+def destroy_segment(master, segment_name):
+
+destroy_segment(master, segment_name)
+Destroys topology segment. First argument should be
object of
class
Host

Instead of description of params as first, second etc., you
may use
following:

+def destroy_segment(master, segment_name):
+
+Destroys topology segment.
+:param master: reference to master object of class Host
+:param segment: name fo segment
and eventually this in other methods
+:returns: Lorem ipsum sit dolor mit amet
+:raises NotFound: if segment is not found


Fixed


4)

cls.replicas[:len(cls.replicas) - 1],

I suggest cls.replicas[:-1]

In [2]: a = [1, 2, 3, 4, 5]

In [3]: a[:-1]
Out[3]: [1, 2, 3, 4]


Fixed


5)
Why re.findall() and then you just use the first result?
'leftnode': self.leftnode_re.findall(i)[0]

Isn't just re.search() enough?
leftnode_re.search(value).group(1)


in fact
leftnode_re.findall(string)[0]
and
leftnode_re.search(string).group(1),
Are equally bad from the readability point of view. The first
one is
even shorter a bit, so why change? :)


It depends on point of view,  because when I reviewed it
yesterday my
brain raises exception that you are trying to add multiple
values to
single value attribute, because you used findall, I expected
that you
need multiple values, and then I saw that index [0] at the end,
and I
was pretty confused what are you trying to achieve.

And because findall is not effective in case when you need to
find just
one occurrence.


I got it. Fixed.









Python 3 nitpick:
I'm not sure if time when we should enforce python 2/3
compability
already comes, but just for record:
instead of open(file, 'r'), please use io.open(file, 

Re: [Freeipa-devel] [PATCH] First part of integration tests for Topology Plugin

2015-08-26 Thread Martin Basti



On 08/26/2015 02:53 PM, Oleg Fayans wrote:

Hi,

No more short links :)

On 08/26/2015 11:50 AM, Tomas Babej wrote:



On 08/26/2015 11:44 AM, Oleg Fayans wrote:

Hi Martin,

On 08/20/2015 11:18 AM, Martin Basti wrote:



On 08/20/2015 10:26 AM, Martin Basti wrote:



On 08/19/2015 04:17 PM, Martin Basti wrote:

I got this:

https://paste.fedoraproject.org/256746/43999380/

FYI replica install failure. (I will retest it, but I'm pretty sure
that it was clean VM, test for some reason install client first)

   File
/usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py, 



line 295, in decorated
 func(installer)
   File
/usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py, 



line 319, in install_check
 sys.exit(IPA client is already configured on this system.\n

2015-08-19T14:14:15Z DEBUG The ipa-replica-install command failed,
exception: SystemExit: IPA client is already configured on this 
system.

Please uninstall it first before configuring the replica, using
'ipa-client-install --uninstall'.
2015-08-19T14:14:15Z ERROR IPA client is already configured on this
system.
Please uninstall it first before configuring the replica, using
'ipa-client-install --uninstall'.

Confirm I got same error.

Fixed. It was a regex error.





On 08/19/2015 09:00 AM, Oleg Fayans wrote:

Hi Martin,

As discussed, here is a new version with pep8-related fixes


On 08/14/2015 10:44 AM, Oleg Fayans wrote:

Hi Martin,

Already noticed that. Implemented the named groups as Tomas 
advised.

Added the third test for
http://www.freeipa.org/page/V4/Manage_replication_topology/Test_plan#Test_case:_Removal_of_a_topology_segment_is_allowed_only_if_there_is_at_least_one_more_segment_connecting_the_given_replica 








On 08/13/2015 05:06 PM, Martin Basti wrote:



On 08/11/2015 03:36 PM, Oleg Fayans wrote:

Hi Martin,

On 08/11/2015 02:02 PM, Martin Basti wrote:

NACK, comments inline.

On 11/08/15 13:25, Oleg Fayans wrote:

Hi Martin,

Thanks for the review!

On 08/10/2015 07:08 PM, Martin Basti wrote:

Thank you for patch, I have a few nitpicks:

1)
On 10/08/15 13:05, Oleg Fayans wrote:

+def create_segment(master, leftnode, rightnode):
+create_segment(master, leftnode, rightnode)

Why do you add the name of method in docstring?

My bad, fixed.


still there

+tokenize_topologies(command_output)
+takes an output of `ipa topologysegment-find` and
returns an
array of


Fixed, sorry.





2)

+def create_segment(master, leftnode, rightnode):
+create_segment(master, leftnode, rightnode)
+creates a topology segment. The first argument is a 
node to

run the
command on
+The first 3 arguments should be objects of class Host
+Returns a hash object containing segment's name, 
leftnode,

rightnode information
+

I would prefer to add assert there instead of just document
that a
Host
object is needed
assert(isinstance(master, Host))
...


Fixed. Created a decorator that checks the type of arguments


This does not scale well.
If we will want to add new argument that is not host object, 
you

need
change it again.


Agreed. Modified the decorator so that you can specify a 
slice of

arguments to be checked and a class to compare to. This does
scale :)


This might be used as function with specified variables that
have to be
host objects




3)
+def destroy_segment(master, segment_name):
+
+destroy_segment(master, segment_name)
+Destroys topology segment. First argument should be
object of
class
Host

Instead of description of params as first, second etc., you
may use
following:

+def destroy_segment(master, segment_name):
+
+Destroys topology segment.
+:param master: reference to master object of class Host
+:param segment: name fo segment
and eventually this in other methods
+:returns: Lorem ipsum sit dolor mit amet
+:raises NotFound: if segment is not found


Fixed


4)

cls.replicas[:len(cls.replicas) - 1],

I suggest cls.replicas[:-1]

In [2]: a = [1, 2, 3, 4, 5]

In [3]: a[:-1]
Out[3]: [1, 2, 3, 4]


Fixed


5)
Why re.findall() and then you just use the first result?
'leftnode': self.leftnode_re.findall(i)[0]

Isn't just re.search() enough?
leftnode_re.search(value).group(1)


in fact
leftnode_re.findall(string)[0]
and
leftnode_re.search(string).group(1),
Are equally bad from the readability point of view. The first
one is
even shorter a bit, so why change? :)


It depends on point of view,  because when I reviewed it
yesterday my
brain raises exception that you are trying to add multiple
values to
single value attribute, because you used findall, I expected
that you
need multiple values, and then I saw that index [0] at the end,
and I
was pretty confused what are you trying to achieve.

And because findall is not effective in case when you need to
find just
one occurrence.


I got it. Fixed.









Python 3 nitpick:
I'm not sure if time when we should enforce python 2/3
compability
already comes, but just 

Re: [Freeipa-devel] [PATCH] First part of integration tests for Topology Plugin

2015-08-20 Thread Martin Basti



On 08/20/2015 10:26 AM, Martin Basti wrote:



On 08/19/2015 04:17 PM, Martin Basti wrote:

I got this:

https://paste.fedoraproject.org/256746/43999380/
FYI replica install failure. (I will retest it, but I'm pretty sure 
that it was clean VM, test for some reason install client first)


  File 
/usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py, 
line 295, in decorated

func(installer)
  File 
/usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py, 
line 319, in install_check

sys.exit(IPA client is already configured on this system.\n

2015-08-19T14:14:15Z DEBUG The ipa-replica-install command failed, 
exception: SystemExit: IPA client is already configured on this system.
Please uninstall it first before configuring the replica, using 
'ipa-client-install --uninstall'.
2015-08-19T14:14:15Z ERROR IPA client is already configured on this 
system.
Please uninstall it first before configuring the replica, using 
'ipa-client-install --uninstall'.

Confirm I got same error.




On 08/19/2015 09:00 AM, Oleg Fayans wrote:

Hi Martin,

As discussed, here is a new version with pep8-related fixes


On 08/14/2015 10:44 AM, Oleg Fayans wrote:

Hi Martin,

Already noticed that. Implemented the named groups as Tomas advised.
Added the third test for
http://www.freeipa.org/page/V4/Manage_replication_topology/Test_plan#Test_case:_Removal_of_a_topology_segment_is_allowed_only_if_there_is_at_least_one_more_segment_connecting_the_given_replica 






On 08/13/2015 05:06 PM, Martin Basti wrote:



On 08/11/2015 03:36 PM, Oleg Fayans wrote:

Hi Martin,

On 08/11/2015 02:02 PM, Martin Basti wrote:

NACK, comments inline.

On 11/08/15 13:25, Oleg Fayans wrote:

Hi Martin,

Thanks for the review!

On 08/10/2015 07:08 PM, Martin Basti wrote:

Thank you for patch, I have a few nitpicks:

1)
On 10/08/15 13:05, Oleg Fayans wrote:

+def create_segment(master, leftnode, rightnode):
+create_segment(master, leftnode, rightnode)

Why do you add the name of method in docstring?

My bad, fixed.


still there

+tokenize_topologies(command_output)
+takes an output of `ipa topologysegment-find` and 
returns an

array of


Fixed, sorry.





2)

+def create_segment(master, leftnode, rightnode):
+create_segment(master, leftnode, rightnode)
+creates a topology segment. The first argument is a node to
run the
command on
+The first 3 arguments should be objects of class Host
+Returns a hash object containing segment's name, leftnode,
rightnode information
+

I would prefer to add assert there instead of just document 
that a

Host
object is needed
assert(isinstance(master, Host))
...


Fixed. Created a decorator that checks the type of arguments


This does not scale well.
If we will want to add new argument that is not host object, you 
need

change it again.


Agreed. Modified the decorator so that you can specify a slice of
arguments to be checked and a class to compare to. This does 
scale :)


This might be used as function with specified variables that 
have to be

host objects




3)
+def destroy_segment(master, segment_name):
+
+destroy_segment(master, segment_name)
+Destroys topology segment. First argument should be 
object of

class
Host

Instead of description of params as first, second etc., you 
may use

following:

+def destroy_segment(master, segment_name):
+
+Destroys topology segment.
+:param master: reference to master object of class Host
+:param segment: name fo segment
and eventually this in other methods
+:returns: Lorem ipsum sit dolor mit amet
+:raises NotFound: if segment is not found


Fixed


4)

cls.replicas[:len(cls.replicas) - 1],

I suggest cls.replicas[:-1]

In [2]: a = [1, 2, 3, 4, 5]

In [3]: a[:-1]
Out[3]: [1, 2, 3, 4]


Fixed


5)
Why re.findall() and then you just use the first result?
'leftnode': self.leftnode_re.findall(i)[0]

Isn't just re.search() enough?
leftnode_re.search(value).group(1)


in fact
leftnode_re.findall(string)[0]
and
leftnode_re.search(string).group(1),
Are equally bad from the readability point of view. The first 
one is

even shorter a bit, so why change? :)


It depends on point of view,  because when I reviewed it 
yesterday my
brain raises exception that you are trying to add multiple 
values to
single value attribute, because you used findall, I expected 
that you
need multiple values, and then I saw that index [0] at the end, 
and I

was pretty confused what are you trying to achieve.

And because findall is not effective in case when you need to 
find just

one occurrence.


I got it. Fixed.









Python 3 nitpick:
I'm not sure if time when we should enforce python 2/3 
compability

already comes, but just for record:
instead of open(file, 'r'), please use io.open(file, 'r') 
(import io

before)


Done.








1)

+#

empty comment here (several times)


Removed






NACK

you changed it wrong

group() returns everything, you need use group(1) 

Re: [Freeipa-devel] [PATCH] First part of integration tests for Topology Plugin

2015-08-20 Thread Martin Basti



On 08/19/2015 04:17 PM, Martin Basti wrote:

I got this:

https://paste.fedoraproject.org/256746/43999380/
FYI replica install failure. (I will retest it, but I'm pretty sure that 
it was clean VM, test for some reason install client first)


  File 
/usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py, 
line 295, in decorated

func(installer)
  File 
/usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py, 
line 319, in install_check

sys.exit(IPA client is already configured on this system.\n

2015-08-19T14:14:15Z DEBUG The ipa-replica-install command failed, 
exception: SystemExit: IPA client is already configured on this system.
Please uninstall it first before configuring the replica, using 
'ipa-client-install --uninstall'.

2015-08-19T14:14:15Z ERROR IPA client is already configured on this system.
Please uninstall it first before configuring the replica, using 
'ipa-client-install --uninstall'.




On 08/19/2015 09:00 AM, Oleg Fayans wrote:

Hi Martin,

As discussed, here is a new version with pep8-related fixes


On 08/14/2015 10:44 AM, Oleg Fayans wrote:

Hi Martin,

Already noticed that. Implemented the named groups as Tomas advised.
Added the third test for
http://www.freeipa.org/page/V4/Manage_replication_topology/Test_plan#Test_case:_Removal_of_a_topology_segment_is_allowed_only_if_there_is_at_least_one_more_segment_connecting_the_given_replica 






On 08/13/2015 05:06 PM, Martin Basti wrote:



On 08/11/2015 03:36 PM, Oleg Fayans wrote:

Hi Martin,

On 08/11/2015 02:02 PM, Martin Basti wrote:

NACK, comments inline.

On 11/08/15 13:25, Oleg Fayans wrote:

Hi Martin,

Thanks for the review!

On 08/10/2015 07:08 PM, Martin Basti wrote:

Thank you for patch, I have a few nitpicks:

1)
On 10/08/15 13:05, Oleg Fayans wrote:

+def create_segment(master, leftnode, rightnode):
+create_segment(master, leftnode, rightnode)

Why do you add the name of method in docstring?

My bad, fixed.


still there

+tokenize_topologies(command_output)
+takes an output of `ipa topologysegment-find` and 
returns an

array of


Fixed, sorry.





2)

+def create_segment(master, leftnode, rightnode):
+create_segment(master, leftnode, rightnode)
+creates a topology segment. The first argument is a node to
run the
command on
+The first 3 arguments should be objects of class Host
+Returns a hash object containing segment's name, leftnode,
rightnode information
+

I would prefer to add assert there instead of just document that a
Host
object is needed
assert(isinstance(master, Host))
...


Fixed. Created a decorator that checks the type of arguments


This does not scale well.
If we will want to add new argument that is not host object, you 
need

change it again.


Agreed. Modified the decorator so that you can specify a slice of
arguments to be checked and a class to compare to. This does scale :)


This might be used as function with specified variables that have 
to be

host objects




3)
+def destroy_segment(master, segment_name):
+
+destroy_segment(master, segment_name)
+Destroys topology segment. First argument should be object of
class
Host

Instead of description of params as first, second etc., you may 
use

following:

+def destroy_segment(master, segment_name):
+
+Destroys topology segment.
+:param master: reference to master object of class Host
+:param segment: name fo segment
and eventually this in other methods
+:returns: Lorem ipsum sit dolor mit amet
+:raises NotFound: if segment is not found


Fixed


4)

cls.replicas[:len(cls.replicas) - 1],

I suggest cls.replicas[:-1]

In [2]: a = [1, 2, 3, 4, 5]

In [3]: a[:-1]
Out[3]: [1, 2, 3, 4]


Fixed


5)
Why re.findall() and then you just use the first result?
'leftnode': self.leftnode_re.findall(i)[0]

Isn't just re.search() enough?
leftnode_re.search(value).group(1)


in fact
leftnode_re.findall(string)[0]
and
leftnode_re.search(string).group(1),
Are equally bad from the readability point of view. The first 
one is

even shorter a bit, so why change? :)


It depends on point of view,  because when I reviewed it 
yesterday my

brain raises exception that you are trying to add multiple values to
single value attribute, because you used findall, I expected that 
you
need multiple values, and then I saw that index [0] at the end, 
and I

was pretty confused what are you trying to achieve.

And because findall is not effective in case when you need to 
find just

one occurrence.


I got it. Fixed.









Python 3 nitpick:
I'm not sure if time when we should enforce python 2/3 compability
already comes, but just for record:
instead of open(file, 'r'), please use io.open(file, 'r') 
(import io

before)


Done.








1)

+#

empty comment here (several times)


Removed






NACK

you changed it wrong

group() returns everything, you need use group(1) to return content in
braces.














-- 
Manage your subscription for the 

Re: [Freeipa-devel] [PATCH] First part of integration tests for Topology Plugin

2015-08-19 Thread Martin Basti

I got this:

https://paste.fedoraproject.org/256746/43999380/

On 08/19/2015 09:00 AM, Oleg Fayans wrote:

Hi Martin,

As discussed, here is a new version with pep8-related fixes


On 08/14/2015 10:44 AM, Oleg Fayans wrote:

Hi Martin,

Already noticed that. Implemented the named groups as Tomas advised.
Added the third test for
http://www.freeipa.org/page/V4/Manage_replication_topology/Test_plan#Test_case:_Removal_of_a_topology_segment_is_allowed_only_if_there_is_at_least_one_more_segment_connecting_the_given_replica 






On 08/13/2015 05:06 PM, Martin Basti wrote:



On 08/11/2015 03:36 PM, Oleg Fayans wrote:

Hi Martin,

On 08/11/2015 02:02 PM, Martin Basti wrote:

NACK, comments inline.

On 11/08/15 13:25, Oleg Fayans wrote:

Hi Martin,

Thanks for the review!

On 08/10/2015 07:08 PM, Martin Basti wrote:

Thank you for patch, I have a few nitpicks:

1)
On 10/08/15 13:05, Oleg Fayans wrote:

+def create_segment(master, leftnode, rightnode):
+create_segment(master, leftnode, rightnode)

Why do you add the name of method in docstring?

My bad, fixed.


still there

+tokenize_topologies(command_output)
+takes an output of `ipa topologysegment-find` and returns an
array of


Fixed, sorry.





2)

+def create_segment(master, leftnode, rightnode):
+create_segment(master, leftnode, rightnode)
+creates a topology segment. The first argument is a node to
run the
command on
+The first 3 arguments should be objects of class Host
+Returns a hash object containing segment's name, leftnode,
rightnode information
+

I would prefer to add assert there instead of just document that a
Host
object is needed
assert(isinstance(master, Host))
...


Fixed. Created a decorator that checks the type of arguments


This does not scale well.
If we will want to add new argument that is not host object, you need
change it again.


Agreed. Modified the decorator so that you can specify a slice of
arguments to be checked and a class to compare to. This does scale :)


This might be used as function with specified variables that have 
to be

host objects




3)
+def destroy_segment(master, segment_name):
+
+destroy_segment(master, segment_name)
+Destroys topology segment. First argument should be object of
class
Host

Instead of description of params as first, second etc., you may use
following:

+def destroy_segment(master, segment_name):
+
+Destroys topology segment.
+:param master: reference to master object of class Host
+:param segment: name fo segment
and eventually this in other methods
+:returns: Lorem ipsum sit dolor mit amet
+:raises NotFound: if segment is not found


Fixed


4)

cls.replicas[:len(cls.replicas) - 1],

I suggest cls.replicas[:-1]

In [2]: a = [1, 2, 3, 4, 5]

In [3]: a[:-1]
Out[3]: [1, 2, 3, 4]


Fixed


5)
Why re.findall() and then you just use the first result?
'leftnode': self.leftnode_re.findall(i)[0]

Isn't just re.search() enough?
leftnode_re.search(value).group(1)


in fact
leftnode_re.findall(string)[0]
and
leftnode_re.search(string).group(1),
Are equally bad from the readability point of view. The first one is
even shorter a bit, so why change? :)


It depends on point of view,  because when I reviewed it yesterday my
brain raises exception that you are trying to add multiple values to
single value attribute, because you used findall, I expected that you
need multiple values, and then I saw that index [0] at the end, and I
was pretty confused what are you trying to achieve.

And because findall is not effective in case when you need to find 
just

one occurrence.


I got it. Fixed.









Python 3 nitpick:
I'm not sure if time when we should enforce python 2/3 compability
already comes, but just for record:
instead of open(file, 'r'), please use io.open(file, 'r') 
(import io

before)


Done.








1)

+#

empty comment here (several times)


Removed






NACK

you changed it wrong

group() returns everything, you need use group(1) to return content in
braces.










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

Re: [Freeipa-devel] [PATCH] First part of integration tests for Topology Plugin

2015-08-19 Thread Oleg Fayans

Hi Martin,

As discussed, here is a new version with pep8-related fixes


On 08/14/2015 10:44 AM, Oleg Fayans wrote:

Hi Martin,

Already noticed that. Implemented the named groups as Tomas advised.
Added the third test for
http://www.freeipa.org/page/V4/Manage_replication_topology/Test_plan#Test_case:_Removal_of_a_topology_segment_is_allowed_only_if_there_is_at_least_one_more_segment_connecting_the_given_replica




On 08/13/2015 05:06 PM, Martin Basti wrote:



On 08/11/2015 03:36 PM, Oleg Fayans wrote:

Hi Martin,

On 08/11/2015 02:02 PM, Martin Basti wrote:

NACK, comments inline.

On 11/08/15 13:25, Oleg Fayans wrote:

Hi Martin,

Thanks for the review!

On 08/10/2015 07:08 PM, Martin Basti wrote:

Thank you for patch, I have a few nitpicks:

1)
On 10/08/15 13:05, Oleg Fayans wrote:

+def create_segment(master, leftnode, rightnode):
+create_segment(master, leftnode, rightnode)

Why do you add the name of method in docstring?

My bad, fixed.


still there

+tokenize_topologies(command_output)
+takes an output of `ipa topologysegment-find` and returns an
array of


Fixed, sorry.





2)

+def create_segment(master, leftnode, rightnode):
+create_segment(master, leftnode, rightnode)
+creates a topology segment. The first argument is a node to
run the
command on
+The first 3 arguments should be objects of class Host
+Returns a hash object containing segment's name, leftnode,
rightnode information
+

I would prefer to add assert there instead of just document that a
Host
object is needed
assert(isinstance(master, Host))
...


Fixed. Created a decorator that checks the type of arguments


This does not scale well.
If we will want to add new argument that is not host object, you need
change it again.


Agreed. Modified the decorator so that you can specify a slice of
arguments to be checked and a class to compare to. This does scale :)


This might be used as function with specified variables that have to be
host objects




3)
+def destroy_segment(master, segment_name):
+
+destroy_segment(master, segment_name)
+Destroys topology segment. First argument should be object of
class
Host

Instead of description of params as first, second etc., you may use
following:

+def destroy_segment(master, segment_name):
+
+Destroys topology segment.
+:param master: reference to master object of class Host
+:param segment: name fo segment
and eventually this in other methods
+:returns: Lorem ipsum sit dolor mit amet
+:raises NotFound: if segment is not found


Fixed


4)

cls.replicas[:len(cls.replicas) - 1],

I suggest cls.replicas[:-1]

In [2]: a = [1, 2, 3, 4, 5]

In [3]: a[:-1]
Out[3]: [1, 2, 3, 4]


Fixed


5)
Why re.findall() and then you just use the first result?
'leftnode': self.leftnode_re.findall(i)[0]

Isn't just re.search() enough?
leftnode_re.search(value).group(1)


in fact
leftnode_re.findall(string)[0]
and
leftnode_re.search(string).group(1),
Are equally bad from the readability point of view. The first one is
even shorter a bit, so why change? :)


It depends on point of view,  because when I reviewed it yesterday my
brain raises exception that you are trying to add multiple values to
single value attribute, because you used findall, I expected that you
need multiple values, and then I saw that index [0] at the end, and I
was pretty confused what are you trying to achieve.

And because findall is not effective in case when you need to find just
one occurrence.


I got it. Fixed.









Python 3 nitpick:
I'm not sure if time when we should enforce python 2/3 compability
already comes, but just for record:
instead of open(file, 'r'), please use io.open(file, 'r') (import io
before)


Done.








1)

+#

empty comment here (several times)


Removed






NACK

you changed it wrong

group() returns everything, you need use group(1) to return content in
braces.






--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
From c7aecff2f7d09b33a5d806f43598373df79b4751 Mon Sep 17 00:00:00 2001
From: Oleg Fayans ofay...@redhat.com
Date: Wed, 19 Aug 2015 08:57:29 +0200
Subject: [PATCH] Added integration tests for Topology plugin

---
 ipatests/test_integration/tasks.py  | 101 ++---
 ipatests/test_integration/test_topology.py  | 143 
 ipatests/test_ipaserver/test_topology_plugin.py |  14 ++-
 3 files changed, 236 insertions(+), 22 deletions(-)
 create mode 100644 ipatests/test_integration/test_topology.py

diff --git a/ipatests/test_integration/tasks.py b/ipatests/test_integration/tasks.py
index 2b83f274619fcb18bcb87118d7df35a85ce52c1a..2544502a692b3a818e489bd94d8651d46972fa60 100644
--- a/ipatests/test_integration/tasks.py
+++ b/ipatests/test_integration/tasks.py
@@ -40,6 +40,24 @@ from ipatests.test_integration.host import Host
 log = log_mgr.get_logger(__name__)
 
 
+def check_arguments_are(slice, instanceof):
+
+:param: slice - tuple of integers denoting the beginning and the 

Re: [Freeipa-devel] [PATCH] First part of integration tests for Topology Plugin

2015-08-14 Thread Oleg Fayans

Hi Martin,

Already noticed that. Implemented the named groups as Tomas advised.
Added the third test for
http://www.freeipa.org/page/V4/Manage_replication_topology/Test_plan#Test_case:_Removal_of_a_topology_segment_is_allowed_only_if_there_is_at_least_one_more_segment_connecting_the_given_replica



On 08/13/2015 05:06 PM, Martin Basti wrote:



On 08/11/2015 03:36 PM, Oleg Fayans wrote:

Hi Martin,

On 08/11/2015 02:02 PM, Martin Basti wrote:

NACK, comments inline.

On 11/08/15 13:25, Oleg Fayans wrote:

Hi Martin,

Thanks for the review!

On 08/10/2015 07:08 PM, Martin Basti wrote:

Thank you for patch, I have a few nitpicks:

1)
On 10/08/15 13:05, Oleg Fayans wrote:

+def create_segment(master, leftnode, rightnode):
+create_segment(master, leftnode, rightnode)

Why do you add the name of method in docstring?

My bad, fixed.


still there

+tokenize_topologies(command_output)
+takes an output of `ipa topologysegment-find` and returns an
array of


Fixed, sorry.





2)

+def create_segment(master, leftnode, rightnode):
+create_segment(master, leftnode, rightnode)
+creates a topology segment. The first argument is a node to
run the
command on
+The first 3 arguments should be objects of class Host
+Returns a hash object containing segment's name, leftnode,
rightnode information
+

I would prefer to add assert there instead of just document that a
Host
object is needed
assert(isinstance(master, Host))
...


Fixed. Created a decorator that checks the type of arguments


This does not scale well.
If we will want to add new argument that is not host object, you need
change it again.


Agreed. Modified the decorator so that you can specify a slice of
arguments to be checked and a class to compare to. This does scale :)


This might be used as function with specified variables that have to be
host objects




3)
+def destroy_segment(master, segment_name):
+
+destroy_segment(master, segment_name)
+Destroys topology segment. First argument should be object of
class
Host

Instead of description of params as first, second etc., you may use
following:

+def destroy_segment(master, segment_name):
+
+Destroys topology segment.
+:param master: reference to master object of class Host
+:param segment: name fo segment
and eventually this in other methods
+:returns: Lorem ipsum sit dolor mit amet
+:raises NotFound: if segment is not found


Fixed


4)

cls.replicas[:len(cls.replicas) - 1],

I suggest cls.replicas[:-1]

In [2]: a = [1, 2, 3, 4, 5]

In [3]: a[:-1]
Out[3]: [1, 2, 3, 4]


Fixed


5)
Why re.findall() and then you just use the first result?
'leftnode': self.leftnode_re.findall(i)[0]

Isn't just re.search() enough?
leftnode_re.search(value).group(1)


in fact
leftnode_re.findall(string)[0]
and
leftnode_re.search(string).group(1),
Are equally bad from the readability point of view. The first one is
even shorter a bit, so why change? :)


It depends on point of view,  because when I reviewed it yesterday my
brain raises exception that you are trying to add multiple values to
single value attribute, because you used findall, I expected that you
need multiple values, and then I saw that index [0] at the end, and I
was pretty confused what are you trying to achieve.

And because findall is not effective in case when you need to find just
one occurrence.


I got it. Fixed.









Python 3 nitpick:
I'm not sure if time when we should enforce python 2/3 compability
already comes, but just for record:
instead of open(file, 'r'), please use io.open(file, 'r') (import io
before)


Done.








1)

+#

empty comment here (several times)


Removed






NACK

you changed it wrong

group() returns everything, you need use group(1) to return content in
braces.


--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
From 50ff9ba1dfaac5f5c382b6900e791639cb6c7e3e Mon Sep 17 00:00:00 2001
From: Oleg Fayans ofay...@redhat.com
Date: Fri, 14 Aug 2015 10:41:12 +0200
Subject: [PATCH] Added first part of integration tests for topology plugin

---
 ipatests/test_integration/tasks.py  |  66 ++-
 ipatests/test_integration/test_topology.py  | 143 
 ipatests/test_ipaserver/test_topology_plugin.py |  14 ++-
 3 files changed, 215 insertions(+), 8 deletions(-)
 create mode 100644 ipatests/test_integration/test_topology.py

diff --git a/ipatests/test_integration/tasks.py b/ipatests/test_integration/tasks.py
index 2b83f274619fcb18bcb87118d7df35a85ce52c1a..ebdddf60c0b0e0fb662d0e5ee5c9ec55cb88559c 100644
--- a/ipatests/test_integration/tasks.py
+++ b/ipatests/test_integration/tasks.py
@@ -40,6 +40,23 @@ from ipatests.test_integration.host import Host
 log = log_mgr.get_logger(__name__)
 
 
+def check_arguments_are(slice, instanceof):
+
+:param: slice - tuple of integers denoting the beginning and the end
+of argument list to be checked
+:param: instanceof - name of the class the checked arguments should be

Re: [Freeipa-devel] [PATCH] First part of integration tests for Topology Plugin

2015-08-13 Thread Tomas Babej


On 08/13/2015 05:06 PM, Martin Basti wrote:
 
 
 On 08/11/2015 03:36 PM, Oleg Fayans wrote:
 Hi Martin,

 On 08/11/2015 02:02 PM, Martin Basti wrote:
 NACK, comments inline.

 On 11/08/15 13:25, Oleg Fayans wrote:
 Hi Martin,

 Thanks for the review!

 On 08/10/2015 07:08 PM, Martin Basti wrote:
 Thank you for patch, I have a few nitpicks:

 1)
 On 10/08/15 13:05, Oleg Fayans wrote:
 +def create_segment(master, leftnode, rightnode):
 +create_segment(master, leftnode, rightnode)
 Why do you add the name of method in docstring?
 My bad, fixed.

 still there

 +tokenize_topologies(command_output)
 +takes an output of `ipa topologysegment-find` and returns an
 array of

 Fixed, sorry.



 2)

 +def create_segment(master, leftnode, rightnode):
 +create_segment(master, leftnode, rightnode)
 +creates a topology segment. The first argument is a node to
 run the
 command on
 +The first 3 arguments should be objects of class Host
 +Returns a hash object containing segment's name, leftnode,
 rightnode information
 +

 I would prefer to add assert there instead of just document that a
 Host
 object is needed
 assert(isinstance(master, Host))
 ...

 Fixed. Created a decorator that checks the type of arguments

 This does not scale well.
 If we will want to add new argument that is not host object, you need
 change it again.

 Agreed. Modified the decorator so that you can specify a slice of
 arguments to be checked and a class to compare to. This does scale :)

 This might be used as function with specified variables that have to be
 host objects


 3)
 +def destroy_segment(master, segment_name):
 +
 +destroy_segment(master, segment_name)
 +Destroys topology segment. First argument should be object of
 class
 Host

 Instead of description of params as first, second etc., you may use
 following:

 +def destroy_segment(master, segment_name):
 +
 +Destroys topology segment.
 +:param master: reference to master object of class Host
 +:param segment: name fo segment
 and eventually this in other methods
 +:returns: Lorem ipsum sit dolor mit amet
 +:raises NotFound: if segment is not found

 Fixed

 4)

 cls.replicas[:len(cls.replicas) - 1],

 I suggest cls.replicas[:-1]

 In [2]: a = [1, 2, 3, 4, 5]

 In [3]: a[:-1]
 Out[3]: [1, 2, 3, 4]

 Fixed

 5)
 Why re.findall() and then you just use the first result?
 'leftnode': self.leftnode_re.findall(i)[0]

 Isn't just re.search() enough?
 leftnode_re.search(value).group(1)

 in fact
 leftnode_re.findall(string)[0]
 and
 leftnode_re.search(string).group(1),
 Are equally bad from the readability point of view. The first one is
 even shorter a bit, so why change? :)

 It depends on point of view,  because when I reviewed it yesterday my
 brain raises exception that you are trying to add multiple values to
 single value attribute, because you used findall, I expected that you
 need multiple values, and then I saw that index [0] at the end, and I
 was pretty confused what are you trying to achieve.

 And because findall is not effective in case when you need to find just
 one occurrence.

 I got it. Fixed.






 Python 3 nitpick:
 I'm not sure if time when we should enforce python 2/3 compability
 already comes, but just for record:
 instead of open(file, 'r'), please use io.open(file, 'r') (import io
 before)

 Done.





 1)

 +#

 empty comment here (several times)

 Removed


 
 NACK
 
 you changed it wrong
 
 group() returns everything, you need use group(1) to return content in
 braces.
 

I'd suggest using named groups in this case, it leads to clearer
expressions.

See: https://docs.python.org/2/library/re.html , (?Pname...) section.

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


Re: [Freeipa-devel] [PATCH] First part of integration tests for Topology Plugin

2015-08-13 Thread Martin Basti



On 08/11/2015 03:36 PM, Oleg Fayans wrote:

Hi Martin,

On 08/11/2015 02:02 PM, Martin Basti wrote:

NACK, comments inline.

On 11/08/15 13:25, Oleg Fayans wrote:

Hi Martin,

Thanks for the review!

On 08/10/2015 07:08 PM, Martin Basti wrote:

Thank you for patch, I have a few nitpicks:

1)
On 10/08/15 13:05, Oleg Fayans wrote:

+def create_segment(master, leftnode, rightnode):
+create_segment(master, leftnode, rightnode)

Why do you add the name of method in docstring?

My bad, fixed.


still there

+tokenize_topologies(command_output)
+takes an output of `ipa topologysegment-find` and returns an
array of


Fixed, sorry.





2)

+def create_segment(master, leftnode, rightnode):
+create_segment(master, leftnode, rightnode)
+creates a topology segment. The first argument is a node to 
run the

command on
+The first 3 arguments should be objects of class Host
+Returns a hash object containing segment's name, leftnode,
rightnode information
+

I would prefer to add assert there instead of just document that a 
Host

object is needed
assert(isinstance(master, Host))
...


Fixed. Created a decorator that checks the type of arguments


This does not scale well.
If we will want to add new argument that is not host object, you need
change it again.


Agreed. Modified the decorator so that you can specify a slice of 
arguments to be checked and a class to compare to. This does scale :)


This might be used as function with specified variables that have to be
host objects




3)
+def destroy_segment(master, segment_name):
+
+destroy_segment(master, segment_name)
+Destroys topology segment. First argument should be object of 
class

Host

Instead of description of params as first, second etc., you may use
following:

+def destroy_segment(master, segment_name):
+
+Destroys topology segment.
+:param master: reference to master object of class Host
+:param segment: name fo segment
and eventually this in other methods
+:returns: Lorem ipsum sit dolor mit amet
+:raises NotFound: if segment is not found


Fixed


4)

cls.replicas[:len(cls.replicas) - 1],

I suggest cls.replicas[:-1]

In [2]: a = [1, 2, 3, 4, 5]

In [3]: a[:-1]
Out[3]: [1, 2, 3, 4]


Fixed


5)
Why re.findall() and then you just use the first result?
'leftnode': self.leftnode_re.findall(i)[0]

Isn't just re.search() enough?
leftnode_re.search(value).group(1)


in fact
leftnode_re.findall(string)[0]
and
leftnode_re.search(string).group(1),
Are equally bad from the readability point of view. The first one is
even shorter a bit, so why change? :)


It depends on point of view,  because when I reviewed it yesterday my
brain raises exception that you are trying to add multiple values to
single value attribute, because you used findall, I expected that you
need multiple values, and then I saw that index [0] at the end, and I
was pretty confused what are you trying to achieve.

And because findall is not effective in case when you need to find just
one occurrence.


I got it. Fixed.









Python 3 nitpick:
I'm not sure if time when we should enforce python 2/3 compability
already comes, but just for record:
instead of open(file, 'r'), please use io.open(file, 'r') (import io
before)


Done.








1)

+#

empty comment here (several times)


Removed






NACK

you changed it wrong

group() returns everything, you need use group(1) to return content in 
braces.


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


Re: [Freeipa-devel] [PATCH] First part of integration tests for Topology Plugin

2015-08-11 Thread Martin Basti

NACK, comments inline.

On 11/08/15 13:25, Oleg Fayans wrote:

Hi Martin,

Thanks for the review!

On 08/10/2015 07:08 PM, Martin Basti wrote:

Thank you for patch, I have a few nitpicks:

1)
On 10/08/15 13:05, Oleg Fayans wrote:

+def create_segment(master, leftnode, rightnode):
+create_segment(master, leftnode, rightnode)

Why do you add the name of method in docstring?

My bad, fixed.


still there

+tokenize_topologies(command_output)
+takes an output of `ipa topologysegment-find` and returns an array of





2)

+def create_segment(master, leftnode, rightnode):
+create_segment(master, leftnode, rightnode)
+creates a topology segment. The first argument is a node to run the
command on
+The first 3 arguments should be objects of class Host
+Returns a hash object containing segment's name, leftnode,
rightnode information
+

I would prefer to add assert there instead of just document that a Host
object is needed
assert(isinstance(master, Host))
...


Fixed. Created a decorator that checks the type of arguments


This does not scale well.
If we will want to add new argument that is not host object, you need 
change it again.


This might be used as function with specified variables that have to be 
host objects




3)
+def destroy_segment(master, segment_name):
+
+destroy_segment(master, segment_name)
+Destroys topology segment. First argument should be object of class
Host

Instead of description of params as first, second etc., you may use
following:

+def destroy_segment(master, segment_name):
+
+Destroys topology segment.
+:param master: reference to master object of class Host
+:param segment: name fo segment
and eventually this in other methods
+:returns: Lorem ipsum sit dolor mit amet
+:raises NotFound: if segment is not found


Fixed


4)

cls.replicas[:len(cls.replicas) - 1],

I suggest cls.replicas[:-1]

In [2]: a = [1, 2, 3, 4, 5]

In [3]: a[:-1]
Out[3]: [1, 2, 3, 4]


Fixed


5)
Why re.findall() and then you just use the first result?
'leftnode': self.leftnode_re.findall(i)[0]

Isn't just re.search() enough?
leftnode_re.search(value).group(1)


in fact
leftnode_re.findall(string)[0]
and
leftnode_re.search(string).group(1),
Are equally bad from the readability point of view. The first one is 
even shorter a bit, so why change? :)


It depends on point of view,  because when I reviewed it yesterday my 
brain raises exception that you are trying to add multiple values to 
single value attribute, because you used findall, I expected that you 
need multiple values, and then I saw that index [0] at the end, and I 
was pretty confused what are you trying to achieve.


And because findall is not effective in case when you need to find just 
one occurrence.








Python 3 nitpick:
I'm not sure if time when we should enforce python 2/3 compability
already comes, but just for record:
instead of open(file, 'r'), please use io.open(file, 'r') (import io
before)


Done.








1)

+#

empty comment here (several times)

--
Martin Basti

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


Re: [Freeipa-devel] [PATCH] First part of integration tests for Topology Plugin

2015-08-11 Thread Oleg Fayans

Hi Martin,

Thanks for the review!

On 08/10/2015 07:08 PM, Martin Basti wrote:

Thank you for patch, I have a few nitpicks:

1)
On 10/08/15 13:05, Oleg Fayans wrote:

+def create_segment(master, leftnode, rightnode):
+create_segment(master, leftnode, rightnode)

Why do you add the name of method in docstring?

My bad, fixed.



2)

+def create_segment(master, leftnode, rightnode):
+create_segment(master, leftnode, rightnode)
+creates a topology segment. The first argument is a node to run the
command on
+The first 3 arguments should be objects of class Host
+Returns a hash object containing segment's name, leftnode,
rightnode information
+

I would prefer to add assert there instead of just document that a Host
object is needed
assert(isinstance(master, Host))
...


Fixed. Created a decorator that checks the type of arguments



3)
+def destroy_segment(master, segment_name):
+
+destroy_segment(master, segment_name)
+Destroys topology segment. First argument should be object of class
Host

Instead of description of params as first, second etc., you may use
following:

+def destroy_segment(master, segment_name):
+
+Destroys topology segment.
+:param master: reference to master object of class Host
+:param segment: name fo segment
and eventually this in other methods
+:returns: Lorem ipsum sit dolor mit amet
+:raises NotFound: if segment is not found


Fixed


4)

cls.replicas[:len(cls.replicas) - 1],

I suggest cls.replicas[:-1]

In [2]: a = [1, 2, 3, 4, 5]

In [3]: a[:-1]
Out[3]: [1, 2, 3, 4]


Fixed


5)
Why re.findall() and then you just use the first result?
'leftnode': self.leftnode_re.findall(i)[0]

Isn't just re.search() enough?
leftnode_re.search(value).group(1)


in fact
leftnode_re.findall(string)[0]
and
leftnode_re.search(string).group(1),
Are equally bad from the readability point of view. The first one is 
even shorter a bit, so why change? :)





Python 3 nitpick:
I'm not sure if time when we should enforce python 2/3 compability
already comes, but just for record:
instead of open(file, 'r'), please use io.open(file, 'r')  (import io
before)


Done.






--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
From b3f008da590608af0ccbc363bec43523369b18dd Mon Sep 17 00:00:00 2001
From: Oleg Fayans ofay...@redhat.com
Date: Tue, 11 Aug 2015 13:19:12 +0200
Subject: [PATCH] Added first part of integration tests for topology plugin

---
 ipatests/test_integration/tasks.py  |  48 +-
 ipatests/test_integration/test_topology.py  | 119 
 ipatests/test_ipaserver/test_topology_plugin.py |  13 +--
 3 files changed, 172 insertions(+), 8 deletions(-)
 create mode 100644 ipatests/test_integration/test_topology.py

diff --git a/ipatests/test_integration/tasks.py b/ipatests/test_integration/tasks.py
index 2b83f274619fcb18bcb87118d7df35a85ce52c1a..e14bc80ec8fd19641f0a4e4ec6c85953c301dd97 100644
--- a/ipatests/test_integration/tasks.py
+++ b/ipatests/test_integration/tasks.py
@@ -40,6 +40,13 @@ from ipatests.test_integration.host import Host
 log = log_mgr.get_logger(__name__)
 
 
+def check_arguments_are_hosts(func):
+def wrapped(*args):
+for i in args:
+assert isinstance(i, Host), Wrong type: %s: %s % (i, type(i))
+func(*args)
+return(wrapped)
+
 def prepare_host(host):
 if isinstance(host, Host):
 env_filename = os.path.join(host.config.test_dir, 'env.sh')
@@ -243,7 +250,6 @@ def install_replica(master, replica, setup_ca=True, setup_dns=False):
 '--forwarder', replica.config.dns_forwarder
 ])
 replica.run_command(args)
-
 enable_replication_debugging(replica)
 setup_sssd_debugging(replica)
 
@@ -507,7 +513,8 @@ def uninstall_master(host):
   paths.SYSCONFIG_PKI_TOMCAT,
   paths.SYSCONFIG_PKI_TOMCAT_PKI_TOMCAT_DIR,
   paths.VAR_LIB_PKI_TOMCAT_DIR,
-  paths.PKI_TOMCAT],
+  paths.PKI_TOMCAT,
+  paths.REPLICA_INFO_GPG_TEMPLATE % host.hostname],
   raiseonerr=False)
 unapply_fixes(host)
 
@@ -519,6 +526,43 @@ def uninstall_client(host):
  raiseonerr=False)
 unapply_fixes(host)
 
+@check_arguments_are_hosts
+def clean_replication_agreement(master, replica):
+
+Performs `ipa-replica-manage del replica_hostname --force`.
+
+master.run_command(['ipa-replica-manage', 'del', replica.hostname, '--force'])
+
+@check_arguments_are_hosts
+def create_segment(master, leftnode, rightnode):
+
+creates a topology segment. The first argument is a node to run the command on
+:returns: a hash object containing segment's name, leftnode, rightnode information
+
+kinit_admin(master)
+lefthost = leftnode.hostname
+righthost = rightnode.hostname
+segment_name = %s-to-%s % (lefthost, righthost)
+result = master.run_command([ipa, 

Re: [Freeipa-devel] [PATCH] First part of integration tests for Topology Plugin

2015-08-11 Thread Oleg Fayans

Hi Martin,

On 08/11/2015 02:02 PM, Martin Basti wrote:

NACK, comments inline.

On 11/08/15 13:25, Oleg Fayans wrote:

Hi Martin,

Thanks for the review!

On 08/10/2015 07:08 PM, Martin Basti wrote:

Thank you for patch, I have a few nitpicks:

1)
On 10/08/15 13:05, Oleg Fayans wrote:

+def create_segment(master, leftnode, rightnode):
+create_segment(master, leftnode, rightnode)

Why do you add the name of method in docstring?

My bad, fixed.


still there

+tokenize_topologies(command_output)
+takes an output of `ipa topologysegment-find` and returns an
array of


Fixed, sorry.





2)

+def create_segment(master, leftnode, rightnode):
+create_segment(master, leftnode, rightnode)
+creates a topology segment. The first argument is a node to run the
command on
+The first 3 arguments should be objects of class Host
+Returns a hash object containing segment's name, leftnode,
rightnode information
+

I would prefer to add assert there instead of just document that a Host
object is needed
assert(isinstance(master, Host))
...


Fixed. Created a decorator that checks the type of arguments


This does not scale well.
If we will want to add new argument that is not host object, you need
change it again.


Agreed. Modified the decorator so that you can specify a slice of 
arguments to be checked and a class to compare to. This does scale :)


This might be used as function with specified variables that have to be
host objects




3)
+def destroy_segment(master, segment_name):
+
+destroy_segment(master, segment_name)
+Destroys topology segment. First argument should be object of class
Host

Instead of description of params as first, second etc., you may use
following:

+def destroy_segment(master, segment_name):
+
+Destroys topology segment.
+:param master: reference to master object of class Host
+:param segment: name fo segment
and eventually this in other methods
+:returns: Lorem ipsum sit dolor mit amet
+:raises NotFound: if segment is not found


Fixed


4)

cls.replicas[:len(cls.replicas) - 1],

I suggest cls.replicas[:-1]

In [2]: a = [1, 2, 3, 4, 5]

In [3]: a[:-1]
Out[3]: [1, 2, 3, 4]


Fixed


5)
Why re.findall() and then you just use the first result?
'leftnode': self.leftnode_re.findall(i)[0]

Isn't just re.search() enough?
leftnode_re.search(value).group(1)


in fact
leftnode_re.findall(string)[0]
and
leftnode_re.search(string).group(1),
Are equally bad from the readability point of view. The first one is
even shorter a bit, so why change? :)


It depends on point of view,  because when I reviewed it yesterday my
brain raises exception that you are trying to add multiple values to
single value attribute, because you used findall, I expected that you
need multiple values, and then I saw that index [0] at the end, and I
was pretty confused what are you trying to achieve.

And because findall is not effective in case when you need to find just
one occurrence.


I got it. Fixed.









Python 3 nitpick:
I'm not sure if time when we should enforce python 2/3 compability
already comes, but just for record:
instead of open(file, 'r'), please use io.open(file, 'r') (import io
before)


Done.








1)

+#

empty comment here (several times)


Removed




--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
From 3569551e68b4f02a0ea9161c47d8c1ec2bdbf041 Mon Sep 17 00:00:00 2001
From: Oleg Fayans ofay...@redhat.com
Date: Tue, 11 Aug 2015 15:34:38 +0200
Subject: [PATCH] Added first part of integration tests for topology plugin

---
 ipatests/test_integration/tasks.py  |  58 +++-
 ipatests/test_integration/test_topology.py  | 115 
 ipatests/test_ipaserver/test_topology_plugin.py |  13 +--
 3 files changed, 178 insertions(+), 8 deletions(-)
 create mode 100644 ipatests/test_integration/test_topology.py

diff --git a/ipatests/test_integration/tasks.py b/ipatests/test_integration/tasks.py
index 2b83f274619fcb18bcb87118d7df35a85ce52c1a..44b9a78b8a82e721817251f6b0e0bc45a8145a2e 100644
--- a/ipatests/test_integration/tasks.py
+++ b/ipatests/test_integration/tasks.py
@@ -40,6 +40,23 @@ from ipatests.test_integration.host import Host
 log = log_mgr.get_logger(__name__)
 
 
+def check_arguments_are(slice, instanceof):
+
+:param: slice - tuple of integers denoting the beginning and the end
+of argument list to be checked
+:param: instanceof - name of the class the checked arguments should be
+instances of
+Example: @check_arguments_are((1, 3), int) will check that the second
+and third arguments are integers
+
+def wrapper(func):
+def wrapped(*args):
+for i in args[slice[0]:slice[1]]:
+assert isinstance(i, instanceof), Wrong type: %s: %s % (i, type(i))
+func(*args)
+return wrapped
+return wrapper
+
 def prepare_host(host):
 if isinstance(host, Host):
 env_filename = 

Re: [Freeipa-devel] [PATCH] First part of integration tests for Topology Plugin

2015-08-10 Thread Martin Basti

Thank you for patch, I have a few nitpicks:

1)
On 10/08/15 13:05, Oleg Fayans wrote:

+def create_segment(master, leftnode, rightnode):
+create_segment(master, leftnode, rightnode)

Why do you add the name of method in docstring?


2)

+def create_segment(master, leftnode, rightnode):
+create_segment(master, leftnode, rightnode)
+creates a topology segment. The first argument is a node to run the 
command on
+The first 3 arguments should be objects of class Host
+Returns a hash object containing segment's name, leftnode, rightnode 
information
+

I would prefer to add assert there instead of just document that a Host object 
is needed
assert(isinstance(master, Host))
...


3)
+def destroy_segment(master, segment_name):
+
+destroy_segment(master, segment_name)
+Destroys topology segment. First argument should be object of class Host

Instead of description of params as first, second etc., you may use 
following:


+def destroy_segment(master, segment_name):
+
+Destroys topology segment.
+:param master: reference to master object of class Host
+:param segment: name fo segment
and eventually this in other methods
+:returns: Lorem ipsum sit dolor mit amet
+:raises NotFound: if segment is not found


4)

cls.replicas[:len(cls.replicas) - 1],

I suggest cls.replicas[:-1]

In [2]: a = [1, 2, 3, 4, 5]

In [3]: a[:-1]
Out[3]: [1, 2, 3, 4]


5)
Why re.findall() and then you just use the first result?
'leftnode': self.leftnode_re.findall(i)[0]

Isn't just re.search() enough?
leftnode_re.search(value).group(1)


Python 3 nitpick:
I'm not sure if time when we should enforce python 2/3 compability already 
comes, but just for record:
instead of open(file, 'r'), please use io.open(file, 'r')  (import io before)




--
Martin Basti

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


Re: [Freeipa-devel] [PATCH] First part of integration tests for Topology Plugin

2015-08-10 Thread Oleg Fayans

Last update to the patch.

On 08/10/2015 09:41 AM, Oleg Fayans wrote:

Hi all.

Applied pPOP8 requirements to the code, removed unused imports. Please,
disregard the first version of this patch

On 08/10/2015 08:20 AM, Oleg Fayans wrote:

Hi list,

Here are 2 integration tests for topology plugin. Plus I got rid of
using nose in test_ipaserver/test_topology_plugin.py: I am using
@pytest.mark.skipif instead









--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
From 595e9eb014d09c4f34e3d63a3889df6dfd503ea0 Mon Sep 17 00:00:00 2001
From: Oleg Fayans ofay...@redhat.com
Date: Mon, 10 Aug 2015 13:03:20 +0200
Subject: [PATCH] Added first part of integration tests for topology plugin

---
 ipatests/test_integration/tasks.py  |  40 +++-
 ipatests/test_integration/test_topology.py  | 119 
 ipatests/test_ipaserver/test_topology_plugin.py |  12 +--
 3 files changed, 163 insertions(+), 8 deletions(-)
 create mode 100644 ipatests/test_integration/test_topology.py

diff --git a/ipatests/test_integration/tasks.py b/ipatests/test_integration/tasks.py
index 2b83f274619fcb18bcb87118d7df35a85ce52c1a..efd1c343d760ed16785b93de102270b80a0cec6f 100644
--- a/ipatests/test_integration/tasks.py
+++ b/ipatests/test_integration/tasks.py
@@ -243,7 +243,6 @@ def install_replica(master, replica, setup_ca=True, setup_dns=False):
 '--forwarder', replica.config.dns_forwarder
 ])
 replica.run_command(args)
-
 enable_replication_debugging(replica)
 setup_sssd_debugging(replica)
 
@@ -507,7 +506,8 @@ def uninstall_master(host):
   paths.SYSCONFIG_PKI_TOMCAT,
   paths.SYSCONFIG_PKI_TOMCAT_PKI_TOMCAT_DIR,
   paths.VAR_LIB_PKI_TOMCAT_DIR,
-  paths.PKI_TOMCAT],
+  paths.PKI_TOMCAT,
+  paths.REPLICA_INFO_GPG_TEMPLATE % host.hostname],
   raiseonerr=False)
 unapply_fixes(host)
 
@@ -519,6 +519,42 @@ def uninstall_client(host):
  raiseonerr=False)
 unapply_fixes(host)
 
+def clean_replication_agreement(master, replica):
+
+clean_replication_agreement(master, replica):
+Performs `ipa-replica-manage del replica_hostname --force`.
+Both arguments should be objects of class Host
+
+master.run_command(['ipa-replica-manage', 'del', replica.hostname, '--force'])
+
+def create_segment(master, leftnode, rightnode):
+create_segment(master, leftnode, rightnode)
+creates a topology segment. The first argument is a node to run the command on
+The first 3 arguments should be objects of class Host
+Returns a hash object containing segment's name, leftnode, rightnode information
+
+kinit_admin(master)
+lefthost = leftnode.hostname
+righthost = rightnode.hostname
+segment_name = %s-to-%s % (lefthost, righthost)
+result = master.run_command([ipa, topologysegment-add, realm, segment_name,
+   --leftnode=%s % lefthost,
+   --rightnode=%s % righthost])
+return {'leftnode': lefthost,
+'rightnode': righthost,
+'name': segment_name}
+
+def destroy_segment(master, segment_name):
+
+destroy_segment(master, segment_name)
+Destroys topology segment. First argument should be object of class Host
+
+kinit_admin(master)
+return master.run_command([ipa,
+   topologysegment-del,
+   realm,
+   segment_name])
+
 
 def get_topo(name_or_func):
 Get a topology function by name
diff --git a/ipatests/test_integration/test_topology.py b/ipatests/test_integration/test_topology.py
new file mode 100644
index ..f4b24a4580c35b62cdb176d04db2dd91e59db9ab
--- /dev/null
+++ b/ipatests/test_integration/test_topology.py
@@ -0,0 +1,119 @@
+#
+# Copyright (C) 2015  FreeIPA Contributors see COPYING for license
+#
+
+import re
+import time
+from ipatests.test_integration.base import IntegrationTest
+from ipatests.test_integration import tasks
+
+
+class TestTopologyOptions(IntegrationTest):
+num_replicas = 2
+topology = 'star'
+segname_re = re.compile(Segment name: ([\w\.\-]+))
+noentries_re = re.compile(Number of entries returned (\d+))
+leftnode_re = re.compile(Left node: ([\w\.]+))
+rightnode_re = re.compile(Right node: ([\w\.]+))
+connectivity_re = re.compile(Connectivity: ([\w\-]+))
+
+@classmethod
+def install(cls, mh):
+tasks.install_topo(cls.topology, cls.master,
+   cls.replicas[:len(cls.replicas) - 1],
+   cls.clients)
+
+def tokenize_topologies(self, command_output):
+
+tokenize_topologies(command_output)
+takes an output of `ipa topologysegment-find` and returns an array of
+segment hashes
+
+segments =