On 06/07/2013 05:49 PM, Ana Krivokapic wrote:
On 06/07/2013 12:15 PM, Tomas Babej wrote:
On 05/31/2013 12:08 PM, Ana Krivokapic wrote:
Hello,
This patch addresses tickethttps://fedorahosted.org/freeipa/ticket/3635.
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Hi, the patch itself works as it should, I have only some refactoring
comments:
1.) There already is a add_range method that is called from within
trust_add's execute()
and handles some range validation (currently only whether domain's
SID of new and
existing range are equal, my patch 67 add some more).
Your patch introduces a new method validate_range() that is called
from execute(),
which leads to some duplication of the logic, mainly two same calls
to the API (getting
the info about the old range via idrange_show)
I'd rather we keep all the validation in one place, preferably inside
the add_range method
or refactored out of it in the new method that is called only from
add_range().
2.) That being said, other parts of refactoring are nice and make
code more clear, +1.
Tomas
My first instinct was to place this validation in the add_range()
method. However, add_range() is called after the trust itself is
added, and validation introduced by this patch needs to happen before
the trust is added (we need to prevent the addition of trust in case
the validation fails).
As for the code duplication, you are right about that. I tried to
minimize duplication - resulting updated patch attached.
--
Regards,
Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.
While this is a nice improvement from the code repetition point of view,
the patch still has one flaw as I see it - the range validation happens
at two separate places:
Once here(already in the code):
if old_range:
old_dom_sid = old_range['result']['ipanttrusteddomainsid'][0];
if old_dom_sid == dom_sid:
return
raise errors.ValidationError(name=_('range exists'),
error=_('ID range with the same name but different ' \
'domain SID already exists. The ID range for ' \
'the new trusted domain must be created manually.'))
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
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
