[Freeipa-devel] [PATCH] 0130 -- create missing idranges in trust-fetch-domains

[Alexander Bokovoy]

On Tue, 14 Jan 2014, Martin Kosek wrote:
>On 01/14/2014 01:02 PM, Alexander Bokovoy wrote:
>> Hi,
>>
>> attached patch implements missing idranges when new child domains
>> discovered through 'ipa trust-fetch-domains'. This functionality existed
>> in 'ipa trust-add' but was not exposed in the 'ipa trust-fetch-domains'.
>>
>> Additionally, error message is shown in case trust name is incorrect.
>>
>> https://fedorahosted.org/freeipa/ticket/4104
>> https://fedorahosted.org/freeipa/ticket/4111
>>
>
>I did not test the patch, just few observations from reading it:
It is generally wrong to base opinion purely on reading the code (see below why) :)

>1) Why are you moving add_range method from trust object to the module? IMO it
>could be left there, it belongs to the object. Plus, the patch won't be that
>big and easier to backport. add_range can be still referred from other commands
>as "self.obj.add_range", no need to move it.
It was in trust_add class, not in the object. I need it in the other
code and without explicit dependency on the object.

>2) This part looks *very* suspicious:
>
>-        trust = self.api.Command.trust_show(keys[0], raw=True)['result']
>+        try:
>+            trust = self.api.Command.trust_show(keys[0], raw=True)['result']
>+        except AssertionError:
>
>I do not think that AssertionError should be raised and caught in normal
>operation, it should be an exceptional exception raised when the world falls
>apart IMO. I.e. I would rather see some PublicError or Exception based
>exception to be raised in trust_show in that case...
It *is* raised and should be caught because this particular snippet is
to catch situation when wrong trust domain name is passed. Previously
the code simply generated server-side exception which resulted in
'internal error'.

-- 
/ Alexander Bokovoy