[pve-devel] [RFC installer] add button for renewing dhcp lease

[Oguz Bektas]

hi!, comments inline

On Thu, Jan 31, 2019 at 08:18:16AM +0100, Thomas Lamprecht wrote:
> On 1/30/19 3:39 PM, Oguz Bektas wrote:
> > per request on #2066, this is more of a proof-of-concept and not
> > thoroughly tested.
> > (i'm aware the button doesn't look very nice at the current position,
> > planning to change it according to comments)
> 
> I've to agree that it really does not look very nice :D
> 
> > 
> > Signed-off-by: Oguz Bektas <o.bektas at proxmox.com>
> > ---
> >  proxinstall | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/proxinstall b/proxinstall
> > index f2414db..0422caa 100755
> > --- a/proxinstall
> > +++ b/proxinstall
> > @@ -2013,6 +2013,13 @@ sub create_ipconf_view {
> >      $device_cb->set_active(0);
> >      $device_cb->set_visible(1);
> >  
> 
> why putting this chunk of code in the mid of the $device_cb handling stuff?
> I'd rather have it above the label create + pack hunk. Not that positioning
> is so criitical, just do not place it in the middle of something.
agreed, i'll change the positioning to make sure it doesn't disturb the
other (logical) code blocks
> 
> > +    my $dhcp_button = Gtk3::Button->new("Renew DHCP");
> 
> I'd use something like:
> Gtk3::Button->new_from_icon_name('view-refresh', 1);
that actually does look better :D
another idea i had was to put the button not all the way at the bottom,
but maybe a smaller button to the right side of the management interface
combobox. what do you think?
> 
> plus a Tooltip for explain it a bit better for the confused ones:
> $dhcp_button->set_tooltip_text("Retrigger DHCP request on interface");
> 
> (above text may not be ideal)
how about: "Renew DHCP Lease on interface"
> 
> and place it besides the $device_cb
> 
> > +    $dhcp_button->signal_connect(clicked => sub {
> > +	run_command("dhclient -r");
> > +	run_command("dhclient -v");
> 
> so why did you chose this exact command set? Even if this is a POC for you,
> description for why you do things like you do them is wanted.
'dhclient -r' will release the current lease and stop the running
dhclient instance from the pid file. i'll comment it in. (dhclient runs
by default on the iso, so it needs to be stopped first)
> 
> why don't you pass only the currently selected NIC to dhclient? Wouldn't that
> make sense at this stage?
makes sense, i'll add that too.
> 
> 
> > +	create_ipconf_view();
> 
> maybe we should only do this if there where actually changes?
not sure about this one. i think it doesn't make us lose much to
just renew the view, instead of binding it to a check for changes which
might take too long in some situations or environments. what do you
think?
> 
> > +    });
> > +
> >      my $get_device_desc = sub {
> >  	my $iface = shift;
> >  	return "$iface->{name} - $iface->{mac} ($iface->{driver})";
> > @@ -2071,6 +2078,8 @@ sub create_ipconf_view {
> >  
> >      $vbox2->pack_start($maskbox, 0, 0, 2);
> >  
> > +    $vbox2->pack_end($dhcp_button, 0, 0, 2);
> > +
> >      $gateway = $config->{gateway} // $ipconf->{gateway} || '192.168.100.1';
> >  
> >      my $gwbox;
> > 
>