remove direct access to profile #1086

jreidinger · 2020-07-09T07:36:29Z

trello: https://trello.com/c/kCfh7Ylo/1941-5-drop-references-to-profilecurrent

teclator · 2020-07-09T07:53:14Z

src/lib/network/network_autoyast.rb

@@ -144,7 +144,7 @@ def activate_s390_devices(section = nil)
    def configure_hosts(write: false)
      log.info("NetworkAutoYast: Hosts configuration")

-      unless ay_current_profile.key? "host"
+      if ay_current_profile.empty?


This rewrite is strange, originally it was call only if "host" section was present, and now if the profile is empty? at least is what the call and variable name suggest..

teclator · 2020-07-09T07:53:55Z

src/lib/network/network_autoyast.rb

-    # Returns current AY profile in the internal representation
-    #
-    # @return [Hash] hash representing current profile or empty hash
-    def ay_current_profile


But then, there is no ay_current_profile

yep, it is correct. Previously it was typo.

teclator · 2020-07-09T07:54:35Z

src/lib/network/network_autoyast.rb

@@ -144,7 +144,7 @@ def activate_s390_devices(section = nil)
    def configure_hosts(write: false)
      log.info("NetworkAutoYast: Hosts configuration")

-      unless ay_current_profile.key? "host"
+      if ay_current_profile.empty?


Suggested change

if ay_current_profile.empty?

if ay_host_section.empty?

yes, it should just check host section. In fact change should be valid as it do some tricks with hosts config, but if section is empty it should be asme as if it is not there at all, not?

yep, that is what I would expect

teclator · 2020-07-09T07:57:48Z

src/lib/network/network_autoyast.rb

-    # Returns global section of current AY profile
-    def ay_general_section
-      return {} if ay_current_profile["general"].nil?
+    attr_writer :ay_networking_section


I guess we plan to use the Y2Network::AutoinstProfile::NetworkingSection object:

https://github.com/yast/yast-network/blob/master/src/lib/y2network/autoinst_profile/networking_section.rb#L37

To be honest I am not sure and I do not want to break it more then it is already.

Ok, we could do a intermediate step before adapting it completely.

teclator · 2020-07-09T08:02:37Z

src/lib/y2network/clients/auto.rb

@@ -77,6 +77,8 @@ def change
      end

      def import(profile)
+        Yast::NetworkAutoYast.instance.ay_networking_section = profile


Suggested change

Yast::NetworkAutoYast.instance.ay_networking_section = profile

Yast::NetworkAutoYast.instance.ay_networking_section =

AutoinstProfile::NetworkingSection.new_from_hashes(profile)

well, does networking section act like Hash? Otherwise I will have to adapt also rest of code.

Not really, and we probably use some defaults already, but then we have to document it very well to not be confused as we could expect an Y2Network::AutoinstProfile::NetworkingSection object instead of hash and vice versa.

teclator · 2020-07-09T08:04:39Z

src/modules/Host.rb

@@ -159,6 +160,8 @@ def Import(settings)
        set_names(ip, names)
      end

+      NetworkAutoYast.instance.ay_host_section = settings


Similar approach commented below

teclator · 2020-07-09T08:20:44Z

I guess that at some point we could even get rid of the direct check of sections and rely on the imported config. Although there could be exceptions and also host section is independent of networking one, so in general I think that the current approach storing the imported sections in the NetworkAutoyast instance looks ok.

@imobachgs what do you think?

imobachgs · 2020-07-15T15:47:07Z

I guess that at some point we could even get rid of the direct check of sections and rely on the imported config. Although there could be exceptions and also host section is independent of networking one, so in general I think that the current approach storing the imported sections in the NetworkAutoyast instance looks ok.

@imobachgs what do you think?

I agree with your POV, it should be the solution at long-term.

coveralls · 2020-07-16T07:41:48Z

Coverage increased (+0.06%) to 71.528% when pulling 738b2fb on remove_profile_current into 1441831 on master.

teclator · 2020-07-16T08:08:28Z

src/lib/network/network_autoyast.rb

@@ -168,6 +168,12 @@ def keep_net_config?
      ret
    end

+    # setter for networking section. Should be done during import.


Please document the type of the variables.

teclator · 2020-07-16T08:11:27Z

@imobachgs @jreidinger

In general looks good to me, although we have to take in mind removed sections that could be cached in the NetworkAutoyast instance and could be a problem if it is done before auto_setup

teclator · 2020-07-16T08:33:17Z

src/lib/network/network_autoyast.rb

@@ -169,9 +169,11 @@ def keep_net_config?
    end

    # setter for networking section. Should be done during import.
+    # @param [Hash] networking section hash


Suggested change

# @param [Hash] networking section hash

# @param value [Hash] networking section hash

teclator · 2020-07-16T08:33:41Z

src/lib/network/network_autoyast.rb

    attr_writer :ay_networking_section

    # setter for host section. Should be done during import.
+    # @param [Hash] host section hash


Suggested change

# @param [Hash] host section hash

# @param value [Hash] host section hash

teclator

LGTM

yast-bot · 2020-07-16T10:11:56Z

✔️ Public Jenkins job #165 successfully finished
✔️ Created OBS submit request #821263

remove direct access to profile

f62b36a

teclator reviewed Jul 9, 2020

View reviewed changes

jreidinger added 3 commits July 13, 2020 22:34

fix accessors and add doc

651044d

fix typo

8c7e335

changes

74891db

jreidinger added 2 commits July 15, 2020 20:58

add conflict call to ensure new ay

88068c9

make rubocop happy

402acaa

jreidinger marked this pull request as ready for review July 16, 2020 07:27

jreidinger changed the title ~~RFC: remove direct access to profile~~ remove direct access to profile Jul 16, 2020

teclator reviewed Jul 16, 2020

View reviewed changes

document type

738b2fb

jreidinger force-pushed the remove_profile_current branch from 9d00495 to 738b2fb Compare July 16, 2020 09:05

teclator approved these changes Jul 16, 2020

View reviewed changes

jreidinger merged commit 92600cd into master Jul 16, 2020

jreidinger deleted the remove_profile_current branch July 16, 2020 10:08

imobachgs mentioned this pull request Jul 29, 2020

bsc#1174643: do not crash AutoYaST when <host> is present #1093

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove direct access to profile #1086

remove direct access to profile #1086

jreidinger commented Jul 9, 2020

teclator Jul 9, 2020

teclator Jul 9, 2020

jreidinger Jul 9, 2020

teclator Jul 9, 2020

jreidinger Jul 9, 2020

teclator Jul 9, 2020

teclator Jul 9, 2020

jreidinger Jul 9, 2020

teclator Jul 9, 2020

teclator Jul 9, 2020

jreidinger Jul 9, 2020

teclator Jul 9, 2020

teclator Jul 9, 2020

teclator commented Jul 9, 2020

imobachgs commented Jul 15, 2020

coveralls commented Jul 16, 2020 •

edited

teclator Jul 16, 2020 •

edited

teclator commented Jul 16, 2020 •

edited

teclator Jul 16, 2020 •

edited

teclator Jul 16, 2020 •

edited

teclator left a comment

yast-bot commented Jul 16, 2020

	Yast::NetworkAutoYast.instance.ay_networking_section = profile
	Yast::NetworkAutoYast.instance.ay_networking_section =
	AutoinstProfile::NetworkingSection.new_from_hashes(profile)

	# @param [Hash] networking section hash
	# @param value [Hash] networking section hash

	# @param [Hash] host section hash
	# @param value [Hash] host section hash

remove direct access to profile #1086

remove direct access to profile #1086

Conversation

jreidinger commented Jul 9, 2020

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

teclator commented Jul 9, 2020

imobachgs commented Jul 15, 2020

coveralls commented Jul 16, 2020 • edited

teclator Jul 16, 2020 • edited

Choose a reason for hiding this comment

teclator commented Jul 16, 2020 • edited

teclator Jul 16, 2020 • edited

Choose a reason for hiding this comment

teclator Jul 16, 2020 • edited

Choose a reason for hiding this comment

teclator left a comment

Choose a reason for hiding this comment

yast-bot commented Jul 16, 2020

coveralls commented Jul 16, 2020 •

edited

teclator Jul 16, 2020 •

edited

teclator commented Jul 16, 2020 •

edited

teclator Jul 16, 2020 •

edited

teclator Jul 16, 2020 •

edited