* [pve-devel] [PATCH container] setup: make randomize_crontab a state subroutine
@ 2021-05-25 13:15 Oguz Bektas
2021-05-25 13:26 ` Thomas Lamprecht
0 siblings, 1 reply; 4+ messages in thread
From: Oguz Bektas @ 2021-05-25 13:15 UTC (permalink / raw)
To: pve-devel
for consistency in post_create_hook. still works as expected
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;
--
2.20.1
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [pve-devel] [PATCH container] setup: make randomize_crontab a state subroutine
2021-05-25 13:15 [pve-devel] [PATCH container] setup: make randomize_crontab a state subroutine Oguz Bektas
@ 2021-05-25 13:26 ` Thomas Lamprecht
2021-05-26 8:48 ` Oguz Bektas
0 siblings, 1 reply; 4+ messages in thread
From: Thomas Lamprecht @ 2021-05-25 13:26 UTC (permalink / raw)
To: Proxmox VE development discussion, Oguz Bektas
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?
> 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;
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [pve-devel] [PATCH container] setup: make randomize_crontab a state subroutine
2021-05-25 13:26 ` Thomas Lamprecht
@ 2021-05-26 8:48 ` Oguz Bektas
2021-05-26 9:08 ` Thomas Lamprecht
0 siblings, 1 reply; 4+ messages in thread
From: Oguz Bektas @ 2021-05-26 8:48 UTC (permalink / raw)
To: Thomas Lamprecht; +Cc: Proxmox VE development discussion
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?
>
> > 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;
> >
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [pve-devel] [PATCH container] setup: make randomize_crontab a state subroutine
2021-05-26 8:48 ` Oguz Bektas
@ 2021-05-26 9:08 ` Thomas Lamprecht
0 siblings, 0 replies; 4+ messages in thread
From: Thomas Lamprecht @ 2021-05-26 9:08 UTC (permalink / raw)
To: Oguz Bektas, Proxmox VE development discussion
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;
>>>
>>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-05-26 9:08 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-25 13:15 [pve-devel] [PATCH container] setup: make randomize_crontab a state subroutine Oguz Bektas
2021-05-25 13:26 ` Thomas Lamprecht
2021-05-26 8:48 ` Oguz Bektas
2021-05-26 9:08 ` Thomas Lamprecht
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox