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