--- Begin Message ---Hey Aaron, hey Christoph! Thanks for your inputs. I already adjusted the code by Christoph’s suggestions which ensure that we have a proper validation of vlans (jftr, we start at vlan id 2 instead of 1 which is also special) and made the overview pages dynamically representing the given vlan tag id only when present. At this point, I just want to make sure this also fits your expectations to keep the review rounds short.> Also; wondering if we maybe should just hide this if no VLAN tag is set, > no not clutter the summary page unnecessarily? Honestly, I have no better idea than this approach except of a complete refactor. So, I’m not sure if you’re fine with this approach within the GUI part where the HTML ack page would look like this: <tr><td>Management Interface:</td> <td>__interface__</td></tr> __vlan__ <tr><td>Hostname:</td> <td>__hostname__</td></tr> And the content gets set by: my $vlan_row = defined $vlan ? "<tr><td>Management Interface VLAN tag:</td><td>$vlan</td></tr>" : ''; Which then gets pushed and set if present: if let Some(vlan) = self.network.vlan { summary.push(SummaryOption::new( "Management interface VLAN tag", &vlan.to_string(), )); } > Instead of the dot notation, I would prefer the vlan-raw-device, vlan-id > variant. > This allows us to give the interface a name that matches the use, and the > whole config is more explicit. Would this fit to you? my $ifaces = "auto lo\niface lo inet loopback\n\n"; my $ip_version = Proxmox::Install::Config::get_ip_version(); my $ntype = $ip_version == 4 ? 'inet' : 'inet6'; my $cidr = Proxmox::Install::Config::get_cidr(); my $vlan = Proxmox::Install::Config::get_vlan(); my $gateway = Proxmox::Install::Config::get_gateway(); my $ethdev = Proxmox::Install::Config::get_mngmt_nic(); my $bridgedev = "vmbr0"; my $mgmtdev = "management"; if ($iso_env->{cfg}->{bridged_network}) { $ifaces .= "iface $ethdev $ntype manual\n"; $ifaces .= "\nauto $bridgedev\n" . "\tiface $bridgedev $ntype manual\n" . "\tbridge-ports $ethdev\n" . "\tbridge-stp off\n" . "\tbridge-fd 0\n" . "\n" "\nauto $mgmtdev\niface $mgmtdev $ntype static\n" . "\taddress $cidr\n" . "\tgateway $gateway\n" . "\tvlan-id $vlan\n" . "\tvlan-raw-device $bridgedev\n"; } else { $ifaces .= "auto $ethdev\n" . "iface $ethdev $ntype manual\n" . "\n" "auto $mgmtdev\n" . "iface $mgmtdev $ntype static\n" . "\taddress $cidr\n" . "\tgateway $gateway\n" . "\tvlan-id $vlan\n" . "\tvlan-raw-device $ethdev\n"; } Happy to hear your feedback. Thanks!
--- End Message ---
_______________________________________________ pve-devel mailing list [email protected] https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
