Forgot the CC to list... Begin forwarded message:
Date: Tue, 19 Jun 2018 12:43:53 +0200 From: Stoiko Ivanov <[email protected]> To: Thomas Lamprecht <[email protected]> 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 <[email protected]> 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! _______________________________________________ pve-devel mailing list [email protected] https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
