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 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.
Please, do not use third-party URL shorteners from within our source
code.
Tomas
I received 2 errors
___________________________________________________________________________________
TestTopologyOptions.test_add_remove_segment
___________________________________________________________________________________
self = <ipatests.test_integration.test_topology.TestTopologyOptions
object at 0x7f6ab95b3110>
def test_add_remove_segment(self):
"""
Make sure a topology segment can be manually created and
deleted
with the influence on the real topology
Testcase
http://www.freeipa.org/page/V4/Manage_replication_topology/
Test_plan#Test_case:_Basic_CRUD_test
"""
tasks.kinit_admin(self.master)
# Install the second replica
tasks.install_replica(self.master, self.replicas[1],
setup_ca=False,
setup_dns=False)
# turn a star into a ring
segment, err = tasks.create_segment(self.master,
self.replicas[0],
> self.replicas[1])
E TypeError: 'NoneType' object is not iterable
test_integration/test_topology.py:102: TypeError
Maybe tasks.create_segment returns None?
In [3]: a, b = None
---------------------------------------------------------------------------
TypeError Traceback (most recent call
last)
<ipython-input-3-c020227e755d> in <module>()
----> 1 a, b = None
TypeError: 'NoneType' object is not iterable
--------------
self = <pytest_multihost.transport.SSHCommand object at
0x7f399b9ea710>, raiseonerr = True
def wait(self, raiseonerr=True):
"""Wait for the remote process to exit
Raises an excption if the exit code is not 0, unless
raiseonerr is
true.
"""
if self._done:
return self.returncode
self._end_process()
self._done = True
if raiseonerr and self.returncode:
self.log.error('Exit code: %s', self.returncode)
> raise subprocess.CalledProcessError(self.returncode,
self.argv)
E CalledProcessError: Command '['ipa',
'topologysegment-del', 'realm',
'vm-152.abc.idm.lab.eng.brq.redhat.com-to-vm-160.abc.idm.lab.eng.brq.redhat.com']'
returned non-zero exit status 2
Honza found where the problem is.
def check_arguments_are(slice, instanceof):
...
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
there should be
return func(*args)
otherwise None will be always returned
--
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
