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

Thomas Lamprecht t.lamprecht at proxmox.com
Tue Jun 19 14:44:59 CEST 2018

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 => ... }));

More information about the pve-devel mailing list