[Freeipa-devel] [PATCH] 042 Password policy commands do not include cospriority

[Martin Kosek]

On Fri, 2011-04-08 at 10:29 -0400, Rob Crittenden wrote:
> Martin Kosek wrote:
> > On Fri, 2011-04-01 at 13:51 -0400, Rob Crittenden wrote:
> >> Martin Kosek wrote:
> >>> Target branches: master, ipa-2-0
> >>> ---
> >>>
> >>> Most of the pwpolicy_* commands do include cospriority in the result
> >>> and potentially in the attribute rights (--all --rights). Especially
> >>> when --raw output is requested. This patch fixes it for all
> >>> pwpolicy commands.
> >>>
> >>> https://fedorahosted.org/freeipa/ticket/1103
> >>>
> >>
> >> nack. I see a couple of problems.
> >>
> >> You should test for rights before doing the cosentry_show(). If rights
> >> is False then we won't add the data whatever it is so it is more
> >> efficient to exit earlier.
> >
> > We have to call cosentry_show every time (except for the case when we
> > pull data for the global policy) because we read cospriority attribute.
> > But the function was indeed not efficient (it called cosentry_show
> > twice), I rewrote it.
> >
> >>
> >> Same with pwpolicy_name == global_policy_name. I think you should drop
> >> the try/except and make it:
> >>
> >> if not rights or pwpolicy_name == global_policy_name:
> >>       return
> >>
> >> ...
> >>
> >> It should never be the case that the cosentry is not found so I'd let it
> >> fail if that does occur.
> >
> > Fixed.
> >
> >>
> >> I think that keys[-1] can be None so be aware.
> >
> > Fixed.
> >
> >>
> >> You hardcode rights == False in pwpolicy_find(), a good thing. I think
> >> you should add make it explict rights=False and add a comment explaining
> >> that you can't get accessrights with a find.
> >
> > Fixed.
> >
> > Fixed patch attached.
> >
> > Martin
> 
> Looks great, ack.
> 
> rob

Pushed to master, ipa-2-0.