Wrapping LanItems::Items into objects #796

mchf · 2019-04-30T07:33:28Z

Currently you can somehow work with "Add" and "Edit" interface workflows. It is only partially working in sense that it do not crash and handles configuration / user input somehow. Definitely needs some more love but should contain all crucial updates which should be needed for switching to new backend

Follow up of #782

imobachgs

For of all, thanks a lot for all this work. Reviewing your PR has helped me to learn a few things :)

However, IMHO this PR introduces too many changes, but I must admit that it might be tricky to split this into smaller (and not breaking) PRs.

For me, the most interesting part is that you have introduced a Y2Network::InterfaceConfigBuilder
class, which looks like a sort of controller. And yes, I can see the need for that. Moreover, some
methods like LanItems#needFirmwareCurrentItem, which received no arguments in the past, now get a iface argument. That's an improvement.

You have said that the PR will break quite some stuff at this point, and that's something we knew
that could happen. But I am not sure whether we should try an alternative approach like rewriting
the workflow for one kind of interface top-bottom (as @teclator proposed). Then, the ConfigWriter
would take care of writing changes to disk for those interfaces. And, actually, we could write new
widgets for that part (moving slowly from the tangled mess of *Dialog methods :)).

The benefit would be to have separate and clean-code, instead of keep fighting against/patching the old one. We could take advantage of quite some work from this PR (all the reading part,
including hwinfo stuff) and we could start rewriting this part in an incremental way (and keep the whole thing working).

Well, I only wanted to open the discussion. What is your POV? @teclator @mchf @jreidinger

imobachgs · 2019-05-06T15:10:01Z

src/lib/y2network/interfaces.rb

+  #
+  # FIXME: Intended for LanItems::Items separation
+  # proper cleanup is must
+  class Interfaces


I like the idea of having a container for interfaces (as we have in storage-ng), although I would call it InterfacesCollection or InterfacesList.

However, this class is responsible for converting the information from sysconfig into Interface objects too (it receives the result of calling Yast::LanItems.Items) which looks wrong to me. What if tomorrow we want to support any other technology? I think it should receive an array of Interface objects.

Then, who is responsible for building the Interface objects? I think ConfigReader::Sysconfig (or any ancillary class) is.

we can separate this conversion method to another kind of reader for example. I'm not against that.

Renaming from Interfaces to e.g. InterfacesCollection also makes sense.

imobachgs · 2019-05-06T15:28:47Z

src/include/network/lan/complex.rb

+      # Load sysconfig into new backend
+      # It is done here bcs this method is currently called during "yast2 lan"
+      # initialization, so it makes configuration available for all submodules
+      system_config = Y2Network::Config.from(:sysconfig)


Why here and not in Lan.Read?

Lan module should be replaced by Y2Network::Config in the future

place it properly in Lan::Read is a bit tricky

I don't see any benefit from placing it into Lan::Read

teclator · 2019-05-07T07:00:37Z

I basically share the same opinion.

imobachgs · 2019-05-07T08:12:05Z

I've been checking some internals and I am not sure whether it is possible to follow the top-bottom approach we were talking about :(

mchf · 2019-05-07T09:06:06Z

well originally I started with this to sort out my thoughts bcs I was completely unsure where should I start from when turning old internals into shining new ones.

However, somewhere around 25th commit I started to be slightly more optimistic - and it doesn't happen that often to me in y2-network ;-)

In the end I think that, as we are at the very beginning of 15sp2 development, we should be able to fix x86 parts easily and quickly when we go by way of accepting this patch. For example we can keep it living a side of master for while and ask QA team to run this version against current integration tests, and so on. What I'm a bit more scared of is s390, but on the other hand I don't see anything that supper complex what should break. There are some additional dialogs which need to be tested and a difference in device initialization. A part of that everything else should be the same. So again nothing what should not be fixed after one or two sprints of intensive work when we have an s390 machine for playing.

What benefits I see? Better test coverable code (currently some parts are very hardly testable bcs of nasty side effects, spaghetti code, ... you now ;-) and cleaned code. For example during work on this patch I did a grep for LanItems.type to replace this calls and surprisingly found a whole dialog and some other related code which was not used at all and the relevant feature was removed in 2007. And I don't talk about other benefits like clear separation of responsibilities and more readable code.

And in the end I don't see this as a (clean) top to down rewriting. In the end I only took the LanItems::Items hash and replaced it with something else (originally I thought I should put it into a class, but in the end I decided to use newly created Y2Network::Interface and go more deeply) and injected this new structure into old workflow. All other changes then replacing GetType(Items[current]["ifcfg"]) by iface.type, were caused a messy code. For example add workflow in the old code was partly editing LanItems::Items directly, partly storing user input in a separate hash ... simply as it turned to be useful during past years of y2-network evolution.

So to summarize that ... I think that doing "the major" cut is best now (I mean now == before all that alfa, beta, GC phases) otherwise we can simply end with nicely rewritten UI into CWM but still working on top of sucking internals. I also don't see it doable and / or useful to rewrite it for example on per interface type basis (I mean create an object e.g. for wlan and rewrite parts of workflows related to this type into objects).

mchf · 2019-05-07T09:11:53Z

And if you'd ask me how to continue ... Up to now I have been (manually) testing only eth workflows, so now I'd continue with other types. This should create a hints for building a Interface specialization hierarchy.

Some other task which remains and should be done independently on the future of this patch is rewriting udev handling into objects.

imobachgs · 2019-05-07T10:50:35Z

I am still thinking that this PR does too much :), but I can see your points and I think we could work from here. I have started a deeper review, but I would like to set a target and finish (and merges it) as soon as possible so we can start working in parallel. I want this PR to stop growing :)

mchf · 2019-05-07T11:20:22Z

I want this PR to stop growing :)

me too. Only think I plan to add is tests and necessary fixes, no more functionality.

imobachgs · 2019-05-07T11:16:08Z

src/lib/y2network/interface_config_builder.rb

+    # @return [String] type which is intended to be build
+    attr_accessor :type
+
+    include Y2Network::InterfaceDefaults


Why is this in a different module? Are you going to reuse it?

no. It is just bunch of methods moved from old code which I haven't check in depth yet.

src/include/network/lan/address.rb

imobachgs · 2019-05-07T11:55:00Z

src/include/network/lan/address.rb

+        builder.set(option: "ETHERDEVICE", value: Ops.get_string(@settings, "ETHERDEVICE", ""))
+        builder.set(
+          options: "VLANID",
+          value:   Builtins.tostring(Ops.get_integer(@settings, "VLAN_ID", 0))


I would replace the Builtins#tostring with modern Ruby code.

imobachgs · 2019-05-07T11:55:09Z

src/include/network/lan/address.rb

-          @settings,
-          "TUNNEL_SET_OWNER",
-          ""
+      elsif Builtins.contains(["tun", "tap"], builder.type)


Same than above.

imobachgs · 2019-05-07T11:55:25Z

src/include/network/lan/address.rb

+      elsif Builtins.contains(["tun", "tap"], builder.type)
+        builder.set(
+          option: "TUNNEL_SET_OWNER",
+          value:  Ops.get_string(@settings, "TUNNEL_SET_OWNER", "")


Same than above (dropping Ops in this case).

src/lib/y2network/interfaces.rb

imobachgs · 2019-05-07T13:21:12Z

src/lib/y2network/interfaces.rb

+    # Converts old LanItems::Items into internal data format
+    #
+    # @return [Interfaces] a container with available interfaces
+    def from_lan_items(lan_items)


Let's keep it by now. I can move this logic to ConfigReader::Sysconfig (or an ancillary class).

imobachgs · 2019-05-07T13:22:11Z

src/modules/Lan.rb

@@ -98,9 +98,6 @@ def main
      @initialized = false

      @backend = nil
-
-      # Y2Network::Config objects
-      @configs = {}


Not sure whether this is a good idea.

imobachgs · 2019-05-07T14:08:30Z

src/modules/LanItems.rb

-      else
-        newdev["NETMASK"] = @netmask
-      end
+      # newdev["IPADDR"] = @ipaddr


why not remove the code instead of just commeting it?

imobachgs · 2019-05-07T14:12:33Z

src/modules/LanItems.rb

@@ -2154,14 +2100,15 @@ def Commit

      # ZONE uses an empty string as the default ZONE which means that is not
      # the same than not defining the attribute
-      current_map = (GetCurrentMap() || {}).select { |k, v| !v.nil? && (k == "ZONE" || !v.empty?) }
+      iface = config.interfaces.find(builder.name)
+      current_map = (!iface.nil? && iface.configured ? iface.config : {}).select { |k, v| !v.nil? && (k == "ZONE" || !v.empty?) }


This code is pretty hard to read. What about splitting it in different lines?

imobachgs · 2019-05-07T14:14:37Z

Some relevant things to fix:

LanItems.isCurrentDHCP && LanItems.isCurrentHotplug
The cmdline will be broken
LanItems.current_udev_name
Drop LanItems.Rollback calls
Udev handling
Move Interfaces to ConfigReader::Sysconfig
https://github.com/yast/yast-network/pull/796/files#diff-cb38af52173f54fb28e2fa7a934f3557R1419

src/lib/y2network/interface.rb

src/lib/y2network/interface_defaults.rb

src/lib/y2network/config.rb

imobachgs · 2019-05-10T08:16:58Z

src/lib/y2network/interfaces_collection.rb

+    #
+    # @return [Boolean] true when both collections are equal, false otherwise
+    def ==(other)
+      @interfaces - other.interfaces && other.interfaces == @interfaces


Sorry, but I do not get the conditions.

yep, probably something like other.interfaces.sort == @interfaces.sort

src/include/network/lan/address.rb

jreidinger · 2019-05-10T12:15:48Z

src/include/network/lan/address.rb

@@ -1005,12 +960,12 @@ def AddressDialog
        ]
      )

-      wd["IPOIB_MODE"] = ipoib_mode_widget if LanItems.GetCurrentType == "ib"
+      wd["IPOIB_MODE"] = ipoib_mode_widget if builder.type == "ib"


this start smelling to me. This type should be enum or own class. But something for future. As plain text comparison can be silently failing for long type. ( I made types often ).

definitely. But we don't have type hierarchy yet. Just one simple Interface class.

jreidinger · 2019-05-10T12:18:03Z

src/include/network/lan/address.rb

-        LanItems.firewall_zone = firewall_zone.store_permanent if firewalld.installed?
+        builder.set(option: "STARTMODE", value: Ops.get_string(@settings, "STARTMODE", ""))
+        builder.set(option: "MTU", value: Ops.get_string(@settings, "MTU", ""))
+        builder.set(option: "ZONE", value: firewall_zone.store_permanent) if firewalld.installed?


hmm, I am still not much fan of this set which is just hidden builder["STARTMODE"] = Ops.get_string(@settings, "STARTMODE", "") so basically hash like API. I still prefer something like builder.startmode = Ops.get_string(@settings, "STARTMODE", "")

no problem with that, already done something similar for the Interface and HwInfo classes and make sense even here. I just wanted isolate all cases where the builder should be used instead of (e.g.) direct access to LanItems internals. For that purpose was hashlike api enough

jreidinger · 2019-05-10T12:19:38Z

src/include/network/lan/address.rb

-        LanItems.vlan_id = Builtins.tostring(
-          Ops.get_integer(@settings, "VLAN_ID", 0)
+      if builder.type == "vlan"
+        builder.set(option: "ETHERDEVICE", value: Ops.get_string(@settings, "ETHERDEVICE", ""))


do we plan to keep this @settings if so, then I would prefer something like builder.load_settings(@settings) which will do this transformation inside.

I currently don't see any benefit in keeping it.

jreidinger · 2019-05-10T13:08:55Z

src/include/network/lan/address.rb

-        "STARTMODE"        => LanItems.startmode,
-        "IFPLUGD_PRIORITY" => LanItems.ifplugd_priority,
+    def initialize_address_settings(builder)
+      @settings.replace(


so in future we will pass builder instead of @settings to CWM widgets, right?

I'd like to. Original idea was to keep "open" interface in it and either submit or drop it as the user wishes (confirms Add / Edit or cancel it)

jreidinger · 2019-05-10T14:04:41Z

src/include/network/lan/complex.rb

+          @builder.name = iface.name
+          @builder.type = iface.type
+          if iface.configured
+            @builder.load_sysconfig(iface.config)


again too much tight to sysconfig. I prefer here generic load. In general I do not like naming here, as it is more like load current config to builder. Not anything about sysconfig.

yes it is. Bcs I'm currently injecting new things into old workflow. And as i currently use sysconfig option names as identifiers in the builder I've picked this name. Definitely something for improving

jreidinger · 2019-05-10T14:05:46Z

src/include/network/lan/complex.rb

+          @builder.type = iface.type
+          if iface.configured
+            @builder.load_sysconfig(iface.config)
+            @builder.load_s390_config(LanItems.s390_ReadQethConfig(iface.hardware.name)) if iface.hardware


I hope that it should be hidden in that generic load.not job of UI to handle different internals for different arch. What is its job if there is specific option for s390, then we need to have specialized UI, but not for loading on s390.

Whole Add / Edit / ... workflow handling needs some polishing as it is somehow spread over the code. We currently need to isolate what is needed to keep ... so thanks for any hint ;-)

jreidinger · 2019-05-10T14:06:48Z

src/include/network/lan/complex.rb

+            @builder.load_sysconfig(iface.config)
+            @builder.load_s390_config(LanItems.s390_ReadQethConfig(iface.hardware.name)) if iface.hardware
+            # FIXME: move to more appropriate place
+            LanItems.operation = :edit


I hope config itself can recognize if it already existed before or not. Maybe something trivial like system_name ( so name before all renames? ) and if it is nil, then it means new and of course nice method for it.

yes ... already planed and described somewhere in this PR. This operation can be identified according to configuration name (I've introduced interface.name to be "persistent" and reflects current state of the system) name exists -> edit action, name do not exist -> add action

jreidinger · 2019-05-10T14:21:05Z

src/include/network/lan/complex.rb

@@ -566,6 +628,8 @@ def MainDialog(init_tab)
        break if input_done?(ret)
      end

+      @builder = nil


is it needed?

most probably not. Just wanted to catch some unexpected accesses.

src/include/network/lan/complex.rb

src/include/network/lan/hardware.rb

jreidinger · 2019-05-10T14:25:29Z

src/include/network/lan/wizards.rb

+        "overview"  => -> { MainDialog("overview", builder: iface_builder) },
+        "add"       => [-> { NetworkCardSequence("add", builder: iface_builder) }, true],
+        "edit"      => [-> { NetworkCardSequence("edit", builder: iface_builder) }, true],
+        "init_s390" => [-> { NetworkCardSequence("init_s390", builder: iface_builder) }, true]


sharing builder accross sequence looks reasonable approach for me.

jreidinger · 2019-05-10T14:38:50Z

src/lib/y2network/hwinfo.rb

+      { name: "wl_channels", default: nil },
+      { name: "wl_bitrates", default: nil }
+    ].each do |hwinfo_item|
+      define_method hwinfo_item[:name].downcase do


why downcase when everything is upcase? or there is keys in hwinfo with upper case? Because this can produce strange looking result like DevName -> devname where you expect dev_name.

only readability ... i prefer obect.method over object.METHOD or so. And I'm also not sure if there is any exception in naming in the hash provided by libhd

jreidinger · 2019-05-10T14:40:53Z

src/lib/y2network/hwinfo.rb

+    alias_method :name, :dev_name
+
+    def exists?
+      !@hwinfo.nil?


to be honest I do not like much this approach with checking everywhere nil. What about raising exception when hwinfo not found and simply return constant with defaults that is defined above? And does it make sense at all having this hwinfo full of nils, false and empty arrays?

i planned to keep only necessary things or load on demand in the future.

Co-Authored-By: imobachgs <igonzalezsosa@suse.com>

Co-Authored-By: Knut Alejandro Anderssen González <kanderssen@suse.de>

mvidner · 2019-05-17T09:07:26Z

src/lib/y2network/hwinfo.rb~HEAD

@@ -0,0 +1,30 @@
+# Copyright (c) [2019] SUSE LLC
+#


This file is a mistake

* Taken from #796.

* Add an InterfacesCollection class * Taken from #796. * Replace Array<Interface> with InterfaceCollection objects

mchf · 2019-07-16T12:34:34Z

Some parts were already merged in other PRs, remaining parts were postponed for now.

mchf force-pushed the replace-items-rebased branch from 3ef571e to 06b4a45 Compare May 3, 2019 09:21

imobachgs reviewed May 6, 2019

View reviewed changes

imobachgs reviewed May 7, 2019

View reviewed changes

teclator reviewed May 8, 2019

View reviewed changes

src/lib/y2network/interface.rb Outdated Show resolved Hide resolved

teclator reviewed May 8, 2019

View reviewed changes

src/lib/y2network/interface_defaults.rb Outdated Show resolved Hide resolved

teclator reviewed May 8, 2019

View reviewed changes

src/lib/y2network/interface_defaults.rb Outdated Show resolved Hide resolved

imobachgs reviewed May 10, 2019

View reviewed changes

src/lib/y2network/config.rb Show resolved Hide resolved

imobachgs reviewed May 10, 2019

View reviewed changes

mchf force-pushed the replace-items-rebased branch from 9337878 to 033546d Compare May 10, 2019 09:08

jreidinger reviewed May 10, 2019

View reviewed changes

src/include/network/lan/address.rb Outdated Show resolved Hide resolved

jreidinger reviewed May 10, 2019

View reviewed changes

src/include/network/lan/complex.rb Outdated Show resolved Hide resolved

jreidinger reviewed May 10, 2019

View reviewed changes

src/include/network/lan/hardware.rb Outdated Show resolved Hide resolved

jreidinger reviewed May 10, 2019

View reviewed changes

imobachgs and others added 20 commits May 17, 2019 10:31

Improve Y2Network::InterfacesCollection documentation

f32b67b

Fix reference to the old Interfaces class

1d5ff5d

Co-Authored-By: imobachgs <igonzalezsosa@suse.com>

Make RubyCop happy

3d16b09

Fix documentation

64a703b

Update src/lib/y2network/interface.rb

3d8b8a8

Co-Authored-By: Knut Alejandro Anderssen González <kanderssen@suse.de>

Rename argument for consistency reasons

87c9879

Eql operator for InterfacesCollection

b5e2cd2

Fixed call for new config write method

130897a

Fixed InterfacesCollection handling in write handler

6c8157c

Minor tweaks based on CR + something for rubocop

0aece5c

Fixes in writing non eth devices

efa7c0f

Dropped LanItems::SetDefaultsForHW

6496a83

Moved devmap intialization from LanItems to interface builder

0192c14

Adapted testsuite

51ae175

Made builder param mandatory

c0bef60

Fixed overview dialog buttons handling

b25a6a3

Fixed overview dialog Delete button

ba1702a

Minor fixes based on CR

f1d25af

Fixed reference to physical / virtual classes

c9dac2c

Fixed testsuite

e0b04f1

mchf force-pushed the replace-items-rebased branch from 8597b47 to e0b04f1 Compare May 17, 2019 08:45

mvidner reviewed May 17, 2019

View reviewed changes

imobachgs added a commit that referenced this pull request May 17, 2019

Add an InterfacesCollection class

350ea0f

* Taken from #796.

mchf added 2 commits May 17, 2019 12:30

Fixes for bridge / bond devices

d59c30d

Removed useless file - remained here after rebasing

64cebdf

imobachgs added a commit that referenced this pull request May 21, 2019

Add an InterfacesCollection class (#813)

c1681b1

* Add an InterfacesCollection class * Taken from #796. * Replace Array<Interface> with InterfaceCollection objects

This was referenced May 29, 2019

Extracted and adapted interface builder #816

Merged

[WIP] Drop replace direct access to LanItems::Items whenever it is possible. #588

Closed

mchf closed this Jul 16, 2019

Wrapping LanItems::Items into objects #796

Wrapping LanItems::Items into objects #796

Conversation

mchf commented Apr 30, 2019 • edited Loading

imobachgs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teclator commented May 7, 2019 • edited Loading

imobachgs commented May 7, 2019

mchf commented May 7, 2019

mchf commented May 7, 2019

imobachgs commented May 7, 2019

mchf commented May 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imobachgs commented May 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mchf commented Jul 16, 2019

mchf commented Apr 30, 2019 •

edited

Loading

teclator commented May 7, 2019 •

edited

Loading