On 25.04.2016 10:39, Stanislav Laznicka wrote:
On 04/22/2016 05:15 PM, Stanislav Laznicka wrote:
On 04/22/2016 01:13 PM, Martin Basti wrote:
On 15.04.2016 14:30, Stanislav Laznicka wrote:
On 04/13/2016 01:40 PM, Martin Basti wrote:
On 06.04.2016 14:04, Stanislav Laznicka wrote:
On 03/30/2016 04:52 PM, Martin Basti wrote:
On 24.03.2016 19:10, 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.
ok
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.
NACK
* ipa-replica-manage refactoring *
1)
Instead of:
- print("Failed to connect to server %s: %s" % (host, e))
- sys.exit(1)
+ root_logger.debug("Failed to connect to server {host}:
{err}"
+ .format(host=host, err=e))
+ raise RuntimeError(e)
I expected
- print("Failed to connect to server %s: %s" % (host, e))
- sys.exit(1)
+ root_logger.debug(traceback.format_exc())
+ raise RuntimeError("Failed to connect to server {host}:
{err}"
+ .format(host=host, err=e)))
Should be fixed now.
2)
- print("No RUV records found.")
- sys.exit(0)
Here is exit state 0, so we should not raise error.
I think that we should create new nonfatal exception.
Made a new nonfatal error exception, then. This step was a bit
controversial when it comes to get_ruv_both_suffixes because it
needs to catch both this new exception and a RuntimeError
exception for both trees. As the main idea here was not to stop
if either tree is missing (resulting in RuntimeError in get_ruv)
or contains no records (NonFatalError), it is only printed that
something bad may have happened (or it's debug-logged in case of
nonfatal errors). In the end, only NonFatalError exception is
raised if no records were found for whatever reason resulting in
the script always returning 0.
3)
- print("unable to decode: %s" % ruv)
+ root_logger.debug("unable to decode: %s" % ruv)
This may break tests, because the logger logs to stderr, leave
it as print for now
4)
- servers = get_ruv(realm, host, dirman_passwd, nolookup)
+ try:
+ servers = get_ruv(realm, host, dirman_passwd, nolookup)
+ except RuntimeError as e:
+ print(e)
+ sys.exit(1)
again we have to print it to stdout for now.
3), 4) done, although it might be better to log to stderr from
patch number 27 and on since the default behavior is changed anyway.
* abort-clean/list/clean-ruv now work for both suffixes *
- if dirman_passwd is None:
+ if dirman_passwd is None or (
+ not dirman_passwd and args[0] in
dirman_passwd_req_commands):
sys.exit("Directory Manager password required")
Done.
Please fix other patch accordingly.
Martin^2
1)
+class NonFatalError(Exception):
+ pass
IMO we should be more specific and call it NoRUVsFound[Exception]
2)
Not sure if this i sstill refactoring, it should be separate patch
- if dirman_passwd is None:
+ if dirman_passwd is None or (
+ not dirman_passwd and args[0] in
dirman_passwd_req_commands):
3)
+def get_ruv_both_suffixes
I think in case where both suffixes returns RuntimeError we should
raise RuntimeError too which results into exit with error code.
Please see the latest patches.
Well, if CA is not installed on replica, it fails, not sure if this
is expected behavior of ipa-replica-manage, or if this is related to
your current patches.
# ipa-replica-manage -p hesloheslo clean-dangling-ruv
Failed to obtain information from 'vm-058-051.example.com': no such
entry
It's actually a bug in clean_dangling_ruvs that was not created in
these patches. I opened a separate ticket for it:
https://fedorahosted.org/freeipa/ticket/5840.
Please see the updated patches. I added verbose flag to
get_ruv_both_suffixes and made list-ruv print that RUVs were not found
in either of the trees instead of empty output for that tree.
ACK
ipa-4-3:
* 41458ab80330cc94ea3d7b1da3fe629b882f4356 replica-manage: fail nicely
when DM psswd required
* cf0fbbae8e2a603424d77d52ccaaf428bca1a233 ipa-replica-manage refactoring
* 1ee1ee2d1ec70d8d15884eafa0472edb63ed70d3 abort-clean/list/clean-ruv
now work for both suffixes
* c5f135bf1b16ea3c27d5b3b62c5b751850e0df70 Moved password check from
clean_dangling_ruv
master:
* 37865aa1d7d388042a3b8a66975b466ec27a3d38 replica-manage: fail nicely
when DM psswd required
* d2bb8b7bb1032bf501c984f26f47ef6778c6796a ipa-replica-manage refactoring
* ee05442e5d65766774c18679af78f21a12309730 abort-clean/list/clean-ruv
now work for both suffixes
* c34af691def03313b61a231b85213c8f20e44cfa Moved password check from
clean_dangling_ruv
--
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