On 06/12/2013 10:23 AM, Alexander Bokovoy wrote:
On Mon, 10 Jun 2013, Ana Krivokapic wrote:
And once here(added by your patch):
+ if not self.validate_range(*keys, **options):
+ raise errors.ValidationError(
+ name=_('id range'),
+ error=_(
+ 'An id range already exists for this trust. '
+ 'You should either delete the old range, or '
+ 'exclude --base-id/--range-size options from the
command.'
I'd suggest we have one common place, say validate_range() function as
we have now,
that contains all the checks (more coming in the future for sure).
That would mean that the whole trust-add command will fail in the case
that "ID range
with the same name but different domain SID already exists", since we
now call validate_range()
within execute() method. I checked with Alexander and we agreed that
this is more desired behaviour.
This would also result to reducement of the number of API calls, which
is a nice benefit.
Tomas
This updated patch completely separates validation logic and object
creation logic of the trust_add command. I added two new methods:
* validate_range(), which ensures that the options and environment
related to idrange is valid, and
* validate_options, which takes care of other general validation
One change introduced in this patch is making the
__populate_remote_domain() method of TrustDomainJoins class public, and
calling it from trust_add. This was necessary in order to enable
discovering details of the trusted domain in validation phase.
Looks good overall but I'd suggest to wrap populate_remote_domain()
calls in join_ad_* methods instead of removing them. If remote domain is
not initialized (i.e.
not(isinstance(self.remote_domain,TrustedDomainInstance)),
then call populate_remote_domain() as it was before.
I tested the patch and it works fine.
There's a lot of refactoring done, but the changes preserve equal state.
Aside from
Alexander's comment up here, I have but one nitpick.
There are few constructs of the form:
try:
value = dictionary['keyword']
except KeyError:
value = None
or
if 'keyword' in dictionary:
value = dictionary['keyword']
else:
value = None
Can you please substitute these with "value = dictionary.get(keyword,
None)"?
Not everywhere, but there are places where it can improve readability of
the code.
All from me for now,
Tomas
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
