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

Move network configuration to the 1st stage by default #606

Merged
merged 7 commits into from
May 22, 2020
Merged

Conversation

teclator
Copy link
Contributor

@teclator teclator commented May 8, 2020

Problem

Historically, AutoYaST has been divided in two stages, doing the installation during the first stage and the configuration during the second.

We have been moving some of the logic to the first stage, but it was still not the default and was only the default when there was no second stage defined.

Solution

Consider the first stage as the default stage for configuring the network. That is, if the networking section is defined in the profile, it will be imported, merging it with the current network config (linuxrc) unless it is specified to not keep the current config (keep_install_network option is defined in the profile as false). Once imported, the section is removed in order to not call the lan_auto client during the second stage.

From here, there are different options that could happen:

  1. Setup the network before the proposal
    The lan_auto client will be called for writing the config
  2. Semiauto network configuration
    The inst_lan client will be called in order to configure the network
  3. Write the network configuration at the end of the installation

While 1 and 2 steps occurs in inst_autosetup(https://github.com/yast/yast-autoinstallation/pull/606/files#diff-0100ef44fa49d992794bb1d6b470d681R168), step 3 is done by save_network client in yast2-network (https://github.com/yast/yast-network/blob/master/src/clients/save_network.rb)

@coveralls
Copy link

coveralls commented May 8, 2020

Coverage Status

Coverage increased (+0.4%) to 35.781% when pulling 0037214 on network_poc into 646b1e1 on master.

@teclator teclator force-pushed the network_poc branch 2 times, most recently from 5bcc551 to 777d392 Compare May 15, 2020 14:29
@teclator teclator changed the title [WIP] Move network configuration to the 1st stage by default Move network configuration to the 1st stage by default May 15, 2020
@teclator teclator marked this pull request as ready for review May 15, 2020 14:37
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.

It looks good. Thanks a lot, because I know this change implies a lot of testing 👏
I only have a minor comment about that #setup_before_proposal method.

On the other hand, I would like to get rid of the references to Yast::Profile (hiding it by now in a #profile method that we can redefine later). But let's keep it simple by now.

if Yast::Profile.current["networking"]
if Yast::Profile.current["networking"]["setup_before_proposal"]
log.info("Networking setup before the proposal")
Yast::AutoinstConfig.network_before_proposal = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the #network_before_proposal method used elsewhere? If that's the case, just keep it as it is. If it is not, I guess we could get rid of it (and use local variable).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will check, it was exposed in the past because could be needed by other modules during the autoconfiguration workflow, like for example save_network.

That information is already stored in Lan when the import is done, so probably we could get rid of it.

But maybe we could do it in a second round. So, finishing with the yast2-network changes related to this first stage configuration first and then deciding where to keep that responsibility.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was introduced by me in: #347

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. Please, make a note somewhere :) and let's continue.

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.

Just a comment regarding the changes file. Otherwise, LGTM.

package/autoyast2.changes Outdated Show resolved Hide resolved
@teclator teclator merged commit daf5ca4 into master May 22, 2020
@teclator teclator deleted the network_poc branch May 22, 2020 08:56
@yast-bot
Copy link
Contributor

✔️ Public Jenkins job #79 successfully finished
✔️ Created OBS submit request #808135

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.

4 participants