[Freeipa-devel] [PATCH 0024] ipa-replica-manage: added --suffix option for certain commands
Stanislav Laznicka
slaznick at redhat.com
Fri Apr 15 12:30:05 UTC 2016
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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-stlaz-0026-3-replica-manage-fail-nicely-when-DM-psswd-required.patch
Type: text/x-patch
Size: 1592 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160415/13db57ae/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-stlaz-0027-3-ipa-replica-manage-refactoring.patch
Type: text/x-patch
Size: 5164 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160415/13db57ae/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-stlaz-0028-3-abort-clean-list-clean-ruv-now-work-for-both-suffixe.patch
Type: text/x-patch
Size: 9626 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160415/13db57ae/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-stlaz-0029-3-Moved-password-check-from-clean_dangling_ruv.patch
Type: text/x-patch
Size: 1827 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160415/13db57ae/attachment-0003.bin>
More information about the Freeipa-devel
mailing list