On 03/30/2016 09:54 AM, Petr Vobornik wrote:
On 03/30/2016 09:37 AM, Stanislav Laznicka wrote:
On 03/24/2016 07:10 PM, Stanislav Laznicka wrote:
On 03/23/2016 08:13 PM, Martin Basti wrote:
[...]
Can you please update design
http://www.freeipa.org/page/V4/Manage_replication_topology_4_4 (mainly
the --suffix option)? Also there are missing clean-ruv and list-ruv
commands in design, and fix usage at the bottom.
1)
I don't understand this expression
+ if dirman_passwd is None or (
+ not dirman_passwd and args[0] in cs_enabled_commands):
You already tested if subcommand belongs to cs_enabled_commands few
lines above, IMO the 'dirman_password is None' expression is enough.
If I understand it well, when empty password is entered, the program
continues
and uses Kerberos credentials for some operations. E.g. for
list-ruv, if empty
password is entered, the command would only display RUVs for domain
tree but
not for the CA tree no matter if CA is set up or not (in the current
state of
the patch, after get_ruv refactoring). This here is one possible way
around
this, although the check for non-empty password might probably just
as well be
in get_ruv_both_suffixes.
2)
+# tuple of commands that work with ca tree and need Directory Manager
password
+cs_enabled_commands = ("list-ruv", "clean-ruv", "abort-clean-ruv")
this variable is used only toi detect if dirman passwd is needed, I
suggest to rename it to commands_req_dirman_passwd, or something
better.
3)
Q: Do we need is_cs_set() function?
A: Yes!
I wanted to give you ultimate NACK, but then I checked how get_ruv
code
works and I changed my mind.
Please write a comment where is_cs_set function is used, why we need
extra function instead of catching an exception, possibly you can
open a
refactoring ticket.
After the refactoring changes, is_cs_set should not be needed
anymore, removed
it.
Shame:
1)
+ if not test_connection(realm, host, options.nolookup) or\
Please use parentheses instead of backslash
2)
+ args[0] in cs_enabled_commands:
+ not dirman_passwd and args[0] in cs_enabled_commands):
Indentation is not multiplication of 4
Shame on me indeed, fixed it.
Nitpicks (I don't insist on fixing these):
1)
+ if servers.get('ca', None):
None is default
2)
+ for (netloc, rid) in servers['ca']:
parentheses are not needed
3)
+ print("\t%s: %s" % (netloc, rid))
Would be nice to use .format() instead of %
Martin^2
I changed my mind, ultimate NACK.
Please fix get_ruv function, is_cs_set will not help. In case there
are no
RUVs but CA is installed, sys.exit there prevents us from removing
RUVs (or
any else operation) on domain suffix, and vice versa.
I propose to move ticket to 4.4 milestone because I would like to
avoid
breaking something in 4.3, as this will hit many places in
ipa-replica-manage.
Please provide the refactoring of get_ruv as separate patch a put
these
patches on top of it.
Martin2
Did the refactoring. There also seemed to be duplicit code in
abort_clean_ruv
for some reason, removed it (I hope it does not break anything, but
it seemed
rather useless). Also had to change the numbers of the patches so
that they
would apply.
Self NACK. As I was updating the design today, I found out I omitted the
information that abort-clean-ruv should now be called with --force by
default.
Updated the arguments of the abort call + man page in the patchset.
I made a mistake in the design page. By --force I actually meant to
use `replica-force-cleaning: yes"` as written in
https://fedorahosted.org/freeipa/ticket/5396 (which means the relevant
ticket in design is wrong).
But #5396 is especially important for clean-dangling ruvs sub command.
I updated the design accordingly, then. The 'almost' original patchset
can therefore be used.
Note: clean-dangling-ruv now uses the --force option on clean-ruv by
default. It may or may not need to be updated later according to how
#5396 is implemented.
--
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
