[Freeipa-devel] [PATCH] enhance ipa-listdelegation

[Rob Crittenden]

Martin Nagy wrote:
> Rob Crittenden wrote:
>> Add options to display a subset of delegations and return 2 if none
>> are found.
>>
>> rob
> 
> Some thoughts:
> 
>>  def parse_options():
>> -    parser = OptionParser()
>> +    usage = "%prog [options]"
>> +    parser = OptionParser(usage)
> 
> No need for usage, "%prog [options]" is the default.

Ok.

> 
>> +    parser.add_option("-s", "--source", dest="source",
>> +                      help="Source group of delegation")
>> +    parser.add_option("-n", "--name", dest="name",
>> +                      help="Name of delegation")
>> +    parser.add_option("-t", "--target", dest="target",
>> +                      help="Target group of delegation")
>>      parser.add_option("--usage", action="store_true",
>>                        help="Program usage")
> 
> I suggest we also remove the --usage option, since it's the same as
> --help if you use parser.print_help()

I thought of that too but for continuity I decided to leave it. With 2.0 
it'll be gone.

> 
>>      parser.add_option("-v", "--verbose", action="store_true",
>> dest="verbose", @@ -56,14 +60,19 @@ def parse_options():
>>      args = ipa.config.init_config(sys.argv)
>>      options, args = parser.parse_args(args)
>>  
>> +    if options.usage or len(args) != 1:
>> +        parser.print_help()
>> +        sys.exit(1)
>> +
> 
> This can be accomplished by parser.error("too many arguments") (without
> the need for sys.exit(1).

Ok.

> 
>> +        if (all or options.name == a.name or
>> +            options.source == group_dn_to_cn[a.source_group] or
>> +            options.target == group_dn_to_cn[a.dest_group]):
> 
> Hm, wouldn't it be better if we required that all conditions are met
> in order to display them?

No, one may want to look at only one delegation, or to see all 
delegations that affect group foo or see all delegations that group bar 
owns. Or they may want to list them all.

> 
>> +            print "Delegation Name: " + a.name
>> +            print "Group " + group_dn_to_cn[a.source_group] 
> 
> Trailing whitespace (it was there before, but still :)

Yeah, the bane of my existence. I still blame Karl.

New patch coming soon.

rob
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 3245 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20080822/f8cbe9b4/attachment.bin>