yast / yast-network

YaST module network
http://en.opensuse.org/Portal:YaST
GNU General Public License v2.0
14 stars 35 forks source link

Copy only the selected backend configuration to the target system (bsc#1206723) #1318

Closed teclator closed 1 year ago

teclator commented 1 year ago

Problem

Until now, the wicked or sysconfig network configuration was always copying to the target system during a normal installation.

Recent changes in the Basesystem filesystem packages removed the /etc/sysconfig/network hierarchy from it not being used by default anymore which raises an exception when YaST tries to merge the dhcp and config files from the instsys.

Solution

Copy only the configuration of the selected backend to the target system.

Testing

coveralls commented 1 year ago

Coverage Status

Coverage: 80.416% (+0.06%) from 80.356% when pulling ffcd36482afc33e191d0eb0e6efac85443bc01c0 on copy_specific_config into 24f3f87b3bea68e945aef574171bf2a4657f2dde on master.

imobachgs commented 1 year ago

In general, these classes encapsulate two different behaviors (which is fine):

Perhaps, we would like to reuse part one only. Let's consider I only want to copy a single file, what about having an API for that (without subclassing)?

copier = Copier.new
copier.copy("/etc/hosts")
copier.copy("/etc/NetworkManager/system-connections", include: ["*.nmconnection"])

Now the Copier class looks more reusable to me. And you can use it even in your NetworkManager or Wicked classes:

class WickedCopier # or Writer or whatever
  def copy
    copier = Copier.new
    copier.copy("/etc/hosts")
    copier.copy("/etc/NetworkManager/system-connections", include: ["*.nmconnection"])
  end
end

WDYT?

teclator commented 1 year ago

Nothing critical, just a few comments about the overall approach because I feel it is influenced by the old code (which is neither good or bad).

Yes, in fact it is an part of the code was almost moved as it is.

yast-bot commented 1 year ago

:heavy_check_mark: Public Jenkins job #317 successfully finished :heavy_check_mark: Created OBS submit request #1059539

yast-bot commented 1 year ago

:heavy_check_mark: Internal Jenkins job #212 successfully finished :heavy_check_mark: Created IBS submit request #288536

mchf commented 1 year ago

Well ... already merged, but as i was asked for an opinion ... I've tested that, read through and it LGTM