On 01/14/2014 01:27 PM, Alexander Bokovoy wrote:
> On Tue, 14 Jan 2014, Martin Kosek wrote:
>> On 01/14/2014 01:02 PM, Alexander Bokovoy wrote:
>>> 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.
>> 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) :)
One does not need to run the code that see the places where it may be rusty :)
>> 1) Why are you moving add_range method from trust object to the module? IMO
>> 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
>> 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.
Ok. Though I would still consider having it rather in the trust object to make
it easier accessible from other modules, though our API system.
>> 2) This part looks *very* suspicious:
>> - trust = self.api.Command.trust_show(keys, raw=True)['result']
>> + try:
>> + trust = self.api.Command.trust_show(keys, 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'.
Ok, understood. My point is, trust_show should not raise AssertException just
because wrong trust domain is passed. There are better means to express that
error - ValidationError or NotFound, based on the situation.
Freeipa-devel mailing list