Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Jan 19, 2023

Conversation

teclator
Copy link
Contributor

@teclator teclator commented Jan 13, 2023

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
Copy link

coveralls commented Jan 13, 2023

Coverage Status

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

Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

src/lib/y2network/config_copier.rb Outdated Show resolved Hide resolved
src/lib/y2network/config_copier.rb Outdated Show resolved Hide resolved
src/lib/y2network/network_manager/config_copier.rb Outdated Show resolved Hide resolved
@imobachgs
Copy link
Contributor

imobachgs commented Jan 16, 2023

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

  • The logic to copy files to the target system (creating the directory, filtering out files, etc.).
  • The list of files that you should copy for each case.

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
Copy link
Contributor Author

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.

@teclator teclator force-pushed the copy_specific_config branch 3 times, most recently from ecbf386 to 793f4a7 Compare January 18, 2023 13:19
@teclator teclator merged commit bd31cb1 into master Jan 19, 2023
@teclator teclator deleted the copy_specific_config branch January 19, 2023 10:50
@yast-bot
Copy link
Contributor

✔️ Public Jenkins job #317 successfully finished
✔️ Created OBS submit request #1059539

@yast-bot
Copy link
Contributor

✔️ Internal Jenkins job #212 successfully finished
✔️ Created IBS submit request #288536

@mchf
Copy link
Member

mchf commented Jan 20, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants