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. TomasThis 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. -- / Alexander Bokovoy _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
