On Wed, 15 Jan 2014, Martin Kosek wrote:
On 01/15/2014 02:54 PM, Sumit Bose wrote:
On Wed, Jan 15, 2014 at 02:14:08PM +0200, Alexander Bokovoy wrote:
On Tue, 14 Jan 2014, Sumit Bose wrote:
On Tue, Jan 14, 2014 at 04:03:06PM +0200, Alexander Bokovoy wrote:
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.

I guess I already hit this while testing. trust-fetch-domains does not
work for valid forest roots anymore :-)

Btw, can you add the forest root check to trustdomain-find as well?
I've fixed it by returning an exception from get_dn(), as it is expected
from other objects' get_dn().

new version attached.

Thanks. I can give functional ACK. All my checks are working as
expected, idranges are added if needed and 'ipa: ERROR: no such entry'
is displayed if the forest name is invalid. Btw I think you can add
https://fedorahosted.org/freeipa/ticket/4095 to the commit message as
well.

The python code looks good to me as well.

bye,
Sumit

I have just few of my usual code purity rants :)

What is the meaning of this?

-            except errors.NotFound:
-                return None
+            except errors.NotFound, e:
+                raise e

Looks like NOOP to me, except it reraises the exception and thus hides the 
origin.
The full try block looks like this:

  try:
     <retrieve from LDAP>
  except errors.NotFound, e:
      raise e
  else:
      <act on the data from LDAP>

The key here is that it is get_dn(). errors.NotFound here has special
meaning and is intercepted by the framework, never checked by the LDAP*
operations (LDAPRetrieve, LDAPSearch, ...). Other exceptions may get
produced and they'll get shown in the logs but errors.NotFound from
get_dn() will never appear in the stacktrace.

The change from 'return None' to re-raising exception is intentional. I
could have dropped the whole 'except ...:' stanza too but this is more of
being explicit in intent here to make clear we want to drop out
exceptionally from get_dn() when entry was not found.

Majority of other get_dn() implementations don't touch LDAP entries at
this point, that makes logical difference here.
--
/ Alexander Bokovoy

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to