[Sle15] AY setup written in first stage #520

mchf · 2017-06-09T09:44:14Z

DNS supported
/etc/hosts supported
Routing supported

coveralls · 2017-06-09T12:39:25Z

Coverage increased (+0.03%) to 29.763% when pulling 05a14a4 on mchf:sle15-ay-network-in-first-stage into 641e67f on yast:master.

coveralls · 2017-06-21T08:22:36Z

Coverage increased (+0.06%) to 29.793% when pulling e03e09a on mchf:sle15-ay-network-in-first-stage into 641e67f on yast:master.

coveralls · 2017-06-21T08:27:00Z

Coverage increased (+0.06%) to 29.793% when pulling 58cc827 on mchf:sle15-ay-network-in-first-stage into 641e67f on yast:master.

coveralls · 2017-06-26T09:00:55Z

Coverage increased (+0.2%) to 29.921% when pulling 45601c4 on mchf:sle15-ay-network-in-first-stage into 641e67f on yast:master.

coveralls · 2017-06-27T06:42:47Z

Coverage increased (+0.2%) to 29.897% when pulling ca7768d on mchf:sle15-ay-network-in-first-stage into 641e67f on yast:master.

mvidner · 2017-06-28T10:10:22Z

package/yast2-network.changes

@@ -1,4 +1,12 @@
 -------------------------------------------------------------------
+Fri Jun  9 09:40:11 UTC 2017 - mfilka@suse.com
+
+- Moving network setup in AY into 1s stage


"1st"
Bug or feature number?

currently not aware of any - the PBI contains no fate number

@kobliha: is there any fate request?

mvidner · 2017-06-28T10:28:43Z

src/clients/save_network.rb

+
+      NetworkAutoYast.instance.configure_nics
+    end
+
    # It does an automatic configuration of installed system


AFAIK formerly this was not associated with AutoYaST. So we should explicitly point this out:

It works for both values of Mode.autoinst

BTW, what about autoupgrade? http://www.rubydoc.info/github/yast/yast-yast2/Yast/ModeClass

upgrade scenario ignores network setup ... but i'm not sure about autoupgrade - will check

mvidner · 2017-06-28T12:11:18Z

src/lib/network/network_autoyast.rb

+        # we neet to guarantee order of the items here
+        [host["host_address"] || "", host["names"] || []]
+      end
+      hosts_config = hosts_config.to_h.delete_if { |k, v| k.empty? || v.empty? }


This non-trivial FromAY-like section should go into its own method (class?) and be covered with tests.

well this is almost everything what configure_hosts does

Ok, now that the other configure_* methods are gone it makes less sense to split it off. Still, with all the "or empty" logic in here, tests are needed.

Also, how does this code relate to Import in host_auto? Do we need to "guarantee item order" also there?

Item order is most probably bad description here. I refer to "host_address" and "names". Each of these hash keys refer to a xml node. And this nodes can be ordered differently in the profile. We must guarantee that "host_address" is at first place.

I think that this duplicates Host.Import . Is that one obsolete then?

Ah, sorry, configure_submodule does call Import

mvidner · 2017-06-28T12:17:33Z

src/modules/DNS.rb

+    #
+    # return {true, false, nil} "yes" => true, "no" => false, otherwise or not
+    # present => nil
+    def write_hostname_to_hosts


A method named write_* that in fact calls SCR.Read?! I know it copies the sysconfig name but here I would prefix it with should_ or something.

mvidner · 2017-06-28T12:23:23Z

src/modules/DNS.rb

-      Builtins.sleep(sl)
+      ProgressNextStage(_("Writing hostname...")) if gui
+      update_hostname
+      Builtins.sleep(0) if gui


Ugh, what?!

hm, consequence of some massive updating and code shifting and so on. I'll drop it.

mvidner · 2017-06-28T12:34:52Z

src/modules/DNS.rb

+        SCR.Write(
+          path(".sysconfig.network.dhcp.WRITE_HOSTNAME_TO_HOSTS"),
+          @write_hostname ? "yes" : "no"
+        ) if !@write_hostname


For use when running first stage mode only

coveralls · 2017-07-11T10:59:26Z

Coverage decreased (-0.1%) to 29.845% when pulling be9825b on mchf:sle15-ay-network-in-first-stage into 465fe0b on yast:master.

coveralls · 2017-07-11T11:10:14Z

Coverage decreased (-0.1%) to 29.85% when pulling 766076d on mchf:sle15-ay-network-in-first-stage into 465fe0b on yast:master.

coveralls · 2017-07-12T08:38:39Z

Coverage decreased (-0.1%) to 29.85% when pulling f7eb359 on mchf:sle15-ay-network-in-first-stage into 465fe0b on yast:master.

mvidner · 2017-07-17T11:36:26Z

does host_auto#Import need similar code like configure_hosts?

mchf · 2017-07-20T08:03:44Z

yes, see https://github.com/yast/yast-network/blob/master/src/clients/host_auto.rb#L86

mvidner · 2017-07-19T08:30:03Z

src/lib/network/network_autoyast.rb

+    #   hosts => [
+    #     # first <host_entry>
+    #     {
+    #       "hosts_address" => <ip>,


No. "host_address".

mvidner · 2017-07-19T08:33:18Z

src/lib/network/network_autoyast.rb

+    end
+
+    # Returns networking section of current AY profile
+    def ay_networking_section


IMHO it would be more clear to replace this method by inlining ay_current_profile.fetch("networking", {})

regarding Host.Import see bellow

in my POV "networking" section is used quite often so it deserved own method as a shortcut.

mvidner · 2017-07-20T08:06:35Z

src/lib/network/network_autoyast.rb

+        # we neet to guarantee order of the items here
+        [host["host_address"] || "", host["names"] || []]
+      end
+      hosts_config = hosts_config.to_h.delete_if { |k, v| k.empty? || v.empty? }


I think that this duplicates Host.Import . Is that one obsolete then?

mvidner

Does this PR mean that the lan_auto+host_auto clients are obsolete, as far as Write is concerned? And they are only useful for the AY config GUI? So /usr/share/YaST2/clients/ayast_setup.rb will no longer work for networking, or work differently ?

mvidner · 2017-07-20T08:23:09Z

src/lib/network/network_autoyast.rb


-      true
+      # expected format for Hosts.Import is { "ip" => [list, of, names] }


OK, and this must be documented there, in the API docs for Host.Import (not Hosts).

mchf · 2017-07-20T09:24:08Z

Writing in first stage is experimental feature from my POV and is used only when second stage is disabled. Othewise _auto clients are used in second stage. See AY integration tests for some more details.

coveralls · 2017-07-20T09:40:11Z

Coverage decreased (-0.2%) to 29.797% when pulling a8d5ee9 on mchf:sle15-ay-network-in-first-stage into 465fe0b on yast:master.

mvidner

I have added tests b2b58c0 , but since you do this in your own repo, there is more work for you to merge them. Wanna push to the shared repo the next time?

mvidner · 2017-07-20T11:38:15Z

package/yast2-network.changes

@@ -1,4 +1,12 @@
 -------------------------------------------------------------------
+Fri Jul 11 09:40:11 UTC 2017 - mfilka@suse.com
+
+- Moving network setup in AY into first stage


As this is a major thing we should have a public bug for this. I could not find one in https://trello.com/c/dxyLJ9c8/956-8-sle-15-%E2%9A%A1-autoyast-configure-the-whole-network-in-the-first-stage so should I create one?
Also, /etc/hosts should be mentioned.

coveralls · 2017-07-20T16:45:33Z

Coverage increased (+0.3%) to 30.241% when pulling 58bb702 on mchf:sle15-ay-network-in-first-stage into 465fe0b on yast:master.

coveralls · 2017-07-21T08:17:44Z

Coverage increased (+0.3%) to 30.241% when pulling b9c5e1a on mchf:sle15-ay-network-in-first-stage into 465fe0b on yast:master.

mchf · 2017-07-21T08:40:35Z

Thanks for the review

mchf force-pushed the sle15-ay-network-in-first-stage branch 2 times, most recently from 79466b0 to 05a14a4 Compare June 9, 2017 12:36

mchf mentioned this pull request Jun 21, 2017

First stage only DNS setup tests yast/aytests-tests#77

Merged

mchf force-pushed the sle15-ay-network-in-first-stage branch from 05a14a4 to e03e09a Compare June 21, 2017 08:19

mchf changed the title ~~[WIP][Sle15] DNS ay setup written in first stage~~ [Sle15] DNS ay setup written in first stage Jun 21, 2017

mchf changed the title ~~[Sle15] DNS ay setup written in first stage~~ [Sle15] AY setup written in first stage Jun 26, 2017

mvidner requested changes Jun 28, 2017

View reviewed changes

mchf added 18 commits July 11, 2017 11:52

Cleanup in DNS::Write - dropped dead code

9c25d6a

Code cleanup: Splitted DNS::Write into smaller pieces

ef87aa3

Enabled DNS::Write in AY's 1st stage

4bcc266

Made DNS::Write in 1st stage dependent on "second_stage" option

be868cb

Improved testsuite for configuring dns in AY's first stage

dc0e9cc

happy rubocop

d1a3ff5

Splitted shared code into private methods in DNS module

d7b3d77

Refactored NetworkAutoYast#configure_dns

40bdff0

Added /etc/hosts config into NetworkAutoYast lib

77ee946

For use when running first stage mode only

Made Host#Write gui aware

4ffc055

Minor fixes + documentation

8423096

Happy rubocop

dad7efe

Support for configuring routing in first stage only AY installation

dd69fbb

Made Routing#Write gui aware

1fc8484

Support for writing NICs configuration in first stage

588c2a4

Use full Lan#Import in first stage only AY run

7a481a5

Cleanup - removed unneccessary helpers from NetworkAutoYast module

5b11294

Documentation

77bfcd8

mchf force-pushed the sle15-ay-network-in-first-stage branch 4 times, most recently from 01e19ae to be9825b Compare July 11, 2017 10:55

Code cleanup

766076d

mchf force-pushed the sle15-ay-network-in-first-stage branch from be9825b to 766076d Compare July 11, 2017 11:06

Fixed typo in changelog

f7eb359

mvidner reviewed Jul 20, 2017

View reviewed changes

Fixed typos, documentation

a8d5ee9

mvidner reviewed Jul 20, 2017

View reviewed changes

Test Lan.FromAY

58bb702

Updated changelog

b9c5e1a

mvidner approved these changes Jul 21, 2017

View reviewed changes

mchf merged commit 02d8e75 into yast:master Jul 21, 2017

mchf deleted the sle15-ay-network-in-first-stage branch July 21, 2017 08:40

mvidner mentioned this pull request Jul 27, 2017

[WIP] Sprint 39 Blog Post Draft yast/yast.github.io#122

Closed


		true
		# expected format for Hosts.Import is { "ip" => [list, of, names] }

[Sle15] AY setup written in first stage #520

[Sle15] AY setup written in first stage #520

Conversation

mchf commented Jun 9, 2017 • edited

coveralls commented Jun 9, 2017

coveralls commented Jun 21, 2017

coveralls commented Jun 21, 2017

coveralls commented Jun 26, 2017

coveralls commented Jun 27, 2017

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

coveralls commented Jul 11, 2017

coveralls commented Jul 11, 2017

coveralls commented Jul 12, 2017

mvidner commented Jul 17, 2017

mchf commented Jul 20, 2017

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

mvidner left a 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 20, 2017

coveralls commented Jul 20, 2017

mvidner left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 20, 2017

coveralls commented Jul 21, 2017

mchf commented Jul 21, 2017

mchf commented Jun 9, 2017 •

edited

mvidner left a comment •

edited