On Tue, 14 Jan 2014, Martin Kosek wrote:
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.
I deliberately don't want that. This is internal API for purposes of
trust code -- do you envision any situation when anyone else might want
to create idranges programmatically for child domains of the existing
trust? Note that you are required to know fairly low-level details of
the AD trusts to call it.
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.
Yes, this is due to the way trust.get_dn() is built. Do not commit this
patch yet (though building packages for testing would be good), I'm
reworking it a bit to move this logic to get_dn() -- otherwise
LDAPRetrieve() will always issue AssertError.
--
/ Alexander Bokovoy
_______________________________________________
Freeipa-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/freeipa-devel
