[Freeipa-devel] [PATCH] 0033 Fail when adding a trust with a different range
Ana Krivokapic
akrivoka at redhat.com
Thu Jun 13 10:17:17 UTC 2013
On 06/12/2013 12:14 PM, Martin Kosek wrote:
> On 06/12/2013 11:40 AM, Tomas Babej wrote:
>> 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.
Fixed.
>> 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.
> You can even use just "value = dictionary.get(keyword)" as None is the default
> return value:
>
>>>> print {}.get("foo")
Fixed.
> None
>
> Martin
>
> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel
Updated patch attached.
--
Regards,
Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-akrivoka-0033-04-Fail-when-adding-a-trust-with-a-different-range.patch
Type: text/x-patch
Size: 14989 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130613/1f406da1/attachment.bin>
More information about the Freeipa-devel
mailing list