[pve-devel] Fw: [PATCH access-control v4 1/2] refactor API by unifying duplicate properties

[Thomas Lamprecht]

On 6/19/18 1:57 PM, Stoiko Ivanov wrote:
> On Mon, 18 Jun 2018 18:23:52 +0200
> Thomas Lamprecht <t.lamprecht at proxmox.com> wrote:
> 
>> On 6/18/18 10:18 AM, Stoiko Ivanov wrote:  
>>> * The JSONSchema for most API calls share some properties, which
>>> this refactoring pulls out in a common_$type_properties hashref in
>>> order to minimize code duplication (All parameters, which are thus
>>> added to the API calls were optional)    
>>
>> OK, but...
>>   
>>> * Additionally for some fields the title property is added (its not
>>> used elswhere) - as preparation for unified CLI handling    
>>
>> ...this is not refactoring...
>>   
>>> * PVE/AccessControl::role_is_special now return 0 instead of '' for
>>> false (Schemavalidation did complain about '')    
>>
>> ...neither this...
>>   
>>> * For 2 fields a new property (print_width) was added    
>>
>> ...and neither this, please do this in separate patches.
>>
>> In general, if you can make separate list points in the commit
>> message, like here, you may consider if splitting the patch up makes
>> it a) easier to digest
>> b) less likely to introduce regressions
>>
>> It's not always straight forward and there's room for opinions but
>> pure refactoring should not be mixed with changes outside of the
>> refactoring scope (in your case, this means all changes which do not
>> directly relate to unifying duplicate properties).  
> good point - I'll send an updated patchset with those 3 points split up
> (and a more fitting commit message, since the common_$type_properties
> hash is not there anymore - sorry for the sloppiness!
> 
> Probably I'll join the addition of title and print_width properties
> into one commit - unless 2 separate patches make sense there for you?
> 

2 patches make sense to me, but one for those "additions only" patches
is here OK too, IMO. So just do what you prefer more :)

>>
>> The end result looks mostly good... another comment inline, though.
>> ..snip..  
>>>  
>>> +sub complete_username {
>>> +
>>> +    my $user_cfg = cfs_read_file('user.cfg');    
>>
>> Hmm, really not sure about this...
>> user.cfg gets registered in PVE::AccessControl so this only works
>> by chance. Also the base Plugin is used (and registered) by
>> PVE::AccessControl, which makes this a somewhat strange dependency
>> cycle...  
> good catch! - I should start reading the code instead of relying on
> rough tests...
> I see the following possibilities (AFAIS PVE::Auth::Plugin doesn't get
> directly included outside of pve-access-control):
> * copy the cfs_register_file into PVE::Auth::Plugin (along with reader
>   and writer sub) - and define the userid option there with the
>   completion sub
> * leave the completion sub in PVE::AccessControl and register the
>   userid option there with the completion sub
> * don't change the definition of the userid option, but add the
>   completion to all get_standard_option calls in User.pm
> I would go for the first option - but maybe I'm overlooking something -
> what do you think?
> 

Hmm, that would let all auth plugins inherit user.cfg parser method,
which does not fit encapsulation very good, IMO, the user.cfg (parsing)
is plugin independent.

An option would be to add a Helper Module which hosts all those
user parse/write and possible other helpers, could be nice.

Or to just don't set the completion key when registering the standard
option, i.e. the last point. Optionally you could register 
 'userid-completed' in the API sub, alá:
register_standard_option(get_standard_option('userid', { completion => ... }));