Re: [pve-devel] [PATCH container] setup: make randomize_crontab a state subroutine

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 26.05.21 10:48, Oguz Bektas wrote:
> On Tue, May 25, 2021 at 03:26:12PM +0200, Thomas Lamprecht wrote:
>> On 25.05.21 15:15, Oguz Bektas wrote:
>>> for consistency in post_create_hook. still works as expected
>>>
>>
>> what's the reason/benefit again?
> 
> it looked a bit ugly otherwise with all the other ones being state subs,
> so it's just a style change in the end. anything against this?

no this is not just a style change! You change a method from being a *private*
helper to be a *public* method, and not only that, as this is a plugin "class"
which other modules base off you added that method to the API of the plugin
interface and that without any actual reason!

Rather than asking other to provide reason against changes ask yourself first
what the actual implications are and if there's any actual advantage, which
"looks prettier in my personal perception" seldom counts towards too...
 
>>
>>> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
>>> ---
>>>  src/PVE/LXC/Setup/Base.pm | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/PVE/LXC/Setup/Base.pm b/src/PVE/LXC/Setup/Base.pm
>>> index d73335b..be41874 100644
>>> --- a/src/PVE/LXC/Setup/Base.pm
>>> +++ b/src/PVE/LXC/Setup/Base.pm
>>> @@ -422,7 +422,7 @@ sub set_user_authorized_ssh_keys {
>>>      $self->ct_modify_file("$home/.ssh/authorized_keys", $ssh_keys, perms => 0700);
>>>  }
>>>  
>>> -my $randomize_crontab = sub {
>>> +sub randomize_crontab {
>>>      my ($self, $conf) = @_;
>>>  
>>>      my @files;
>>> @@ -450,7 +450,7 @@ my $randomize_crontab = sub {
>>>  	}
>>>  	$self->ct_file_set_contents($filename, $new);
>>>     }
>>> -};
>>> +}
>>>  
>>>  sub set_timezone {
>>>      my ($self, $conf) = @_;
>>> @@ -493,7 +493,7 @@ sub post_create_hook {
>>>  
>>>      $self->template_fixup($conf);
>>>  
>>> -    &$randomize_crontab($self, $conf);
>>> +    $self->randomize_crontab($conf);
>>>  
>>>      $self->set_user_password($conf, 'root', $root_password);
>>>      $self->set_user_authorized_ssh_keys($conf, 'root', $ssh_keys) if $ssh_keys;
>>>
>>