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

[Stoiko Ivanov]

Forgot the CC to list...

Begin forwarded message:

Date: Tue, 19 Jun 2018 12:43:53 +0200
From: Stoiko Ivanov <s.ivanov at proxmox.com>
To: Thomas Lamprecht <t.lamprecht at proxmox.com>
Subject: Re: [pve-devel] [PATCH access-control v4 1/2] refactor API by
unifying duplicate properties


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?

> 
> 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?


> 
>..snip..
>   


Thanks for the through review!