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: >>> 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) :)
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 >> 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. 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[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'. 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. Martin _______________________________________________ Freeipa-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/freeipa-devel
