Use the new ServiceWidget #73

dgdavid · 2018-08-01T11:14:46Z

More info: https://trello.com/c/uAe4i9Ru/107-5-fate319428-allow-socket-activation-widget

From	To

teclator · 2018-08-01T12:38:04Z

package/yast2-dns-server.spec

-# SuSEFirewall2 replaced by firewalld
-BuildRequires:  yast2 >= 4.0.39
+# Yast2::ServiceWidget
+BuildRequires:  yast2 >= 4.1.0


Not only required for Building

teclator · 2018-08-01T12:56:01Z

In general, it looks good. Just not sure about the responsibility of restarting services as it was partially in DnsService.Write method.

Other doubt, could you link the PR which contains the yast2-4.1.0 version?

dgdavid · 2018-08-01T13:51:27Z

Just not sure about the responsibility of restarting services as it was partially in DnsService.Write method.

What do you mean? Restart/Reload the service always after write settings?

joseivanlopez · 2018-08-07T08:03:13Z

src/lib/dns-server/service_widget_helpers.rb

@@ -0,0 +1,22 @@
+require "cwm/service_widget"


Please, add license text at the beginning of the file.

joseivanlopez · 2018-08-07T08:08:00Z

src/lib/dns-server/service_widget_helpers.rb

+      @service ||= Yast2::SystemService.find("named")
+    end
+
+    # Returns the status widget for service


Only for curiosity, why do you refer the widget as "status widget"? It confuses me a bit.

Because it is the (service) widget responsible to manage the final status of service (running, stopped, etc). But if it is confusing I could change it and simply use "service widget" :)

joseivanlopez · 2018-08-07T08:09:37Z

src/lib/dns-server/service_widget_helpers.rb

+
+    # Returns the status widget for service
+    #
+    # @return [::CWM::ServiceWidget] service status widget


NP: is it necessary to explicitly add :: for top-level name resolution? In case there is no conflict, I would not add it.

I had problems without the ::, but I can't remember if it was here. However, after that I started to use ::CWM::ServiceWidget in all modules, for consistence. Shall I to use :: only when needed?

I was curious why you needed it. Fine with me as it is now.

joseivanlopez · 2018-08-07T08:12:34Z

src/include/dns-server/dialog-main.rb


 module Yast
  # Representation of the configuration of dns-server.
  # Input and output routines.
  module DnsServerDialogMainInclude
+    include Yast::Logger


I guess it is necessary to require yast to use the logger. Probably it works because another file is already requiring yast.

So, I must to explicitly require "yast" at the beginning, isn't it?

Or require "yast/logger"

requiring yast is quite common.

joseivanlopez · 2018-08-07T08:19:48Z

src/include/dns-server/dialog-main.rb

+      Wizard.RestoreHelp(write_help_text)
+
+      return :next if write_settings
+      return :back if Popup.YesNo(_("Saving the configuration failed. Change the settings?"))


NP: same comment as in another PR. In new code I would use Yast2::Popup.

joseivanlopez · 2018-08-07T08:20:27Z

src/include/dns-server/dialog-main.rb

    end

-    # Writes settings and restores the dialog without exiting
-    def SaveAndRestart
+    # Writes settings without exit


NP: typo "without exit" => "without exiting" ?

joseivanlopez · 2018-08-07T08:24:54Z

src/include/dns-server/dialog-main.rb

      nil
    end

+    # Writes DNS server settings and save the service
+    #
+    # NOTE: currently, the DnsServer is a Perl module, reason why the write of


NP: please, use @note

joseivanlopez · 2018-08-07T08:28:46Z

src/include/dns-server/dialog-main.rb

            VSpacing(),
            "firewall",
            VStretch(),
            Right(
-              PushButton(Id("apply"), _("Apply Changes"))


Why this change? I would tend to avoid the Hash way to define widgets.

joseivanlopez · 2018-08-07T08:30:00Z

src/lib/dns-server/service_widget_helpers.rb

+require "yast2/system_service"
+
+module Y2DnsServer
+  module ServiceWidgetHelpers


NP: Do you think this "helper" pays off? At the end, it only contains to simple methods.

The other option is to have this two simple methods duplicated, reason why I move them here.

joseivanlopez · 2018-08-07T08:50:16Z

src/modules/DnsServer.pm

-	}
-	Service->Disable ("named");
+
+    if (Mode->auto() || Mode->config())


Probably we could definitely remove this code and use SystemService in auto client.

I agree, but maybe it is better to do it as a dedicated task, after finish the ServiceWidget adaptation.

joseivanlopez

Please, update version and add changelog entry.

Since Yast2::SystemService (used through the new ServiceWidget) is responsible to update properly the service status (start/stop/restart/...), related logic in the DnsServer Perl module it has been limited to be executed only for AutoYast (`auto` and `config` modes).

coveralls · 2018-08-10T08:43:23Z

Coverage increased (+0.7%) to 20.475% when pulling fb6a305 on feature/service_widget into 29d565a on master.

Much of the methods related to status service widget were duplicated in both dialog, reason why it has been moved to a helper module.

imobachgs

Some minor comments. Otherwise, it looks good.

imobachgs · 2018-08-10T10:35:07Z

src/include/dns-server/dialog-installwizard.rb

@@ -23,6 +28,14 @@ def initialize_dns_server_dialog_installwizard(include_target)
      Yast.import "CWMFirewallInterfaces"
    end

+    # Writes settings and save the service


imobachgs · 2018-08-10T10:36:03Z

src/include/dns-server/dialog-main.rb


 module Yast
  # Representation of the configuration of dns-server.
  # Input and output routines.
  module DnsServerDialogMainInclude
+    include Yast::Logger


requiring yast is quite common.

imobachgs · 2018-08-10T10:37:12Z

src/include/dns-server/dialog-main.rb

-          :abort
-        end
-      end
+      log.info("Running write dialog")


Do you really want to log this?

No, I don't. However I started with dhcp module, which has an identical method which logs that information. So, in attempt to has an unified code I put it also here.

But no problem deleting it :)

imobachgs · 2018-08-10T10:38:27Z

src/include/dns-server/dialog-main.rb


      nil
    end

+    # Writes DNS server settings and save the service


imobachgs · 2018-08-10T10:38:48Z

src/include/dns-server/dialog-main.rb

+      DnsServer.Write && service.save
+    end
+
+    # Shows a popup asking to user if wants to change settings


np: to the user

imobachgs · 2018-08-10T10:38:58Z

src/include/dns-server/dialog-main.rb

+
+    # Shows a popup asking to user if wants to change settings
+    #
+    # @return [Boolean] true if user decides to go back to change settings; false otherwise


np: the user

imobachgs · 2018-08-10T10:40:05Z

src/include/dns-server/dialog-main.rb

+    #
+    # @return [Boolean] true if user decides to go back to change settings; false otherwise
+    def back_to_change_setting?
+      change_settings_message = _("Saving the configuration failed. Change the settings?")


We are keeping the original text (which is right). But it does not sound good to me. I would go for something like "Do you want to review the settings?". But, for the time being, let's keep it as it is because it is out of the scope of this PR.

imobachgs · 2018-08-10T10:40:33Z

src/include/dns-server/dialog-main.rb

+    # Validates and saves CWM widgets
+    #
+    # @param [Hash] event map that triggered saving
+    def validate_and_save_widgets(event)


Please, document the return value.

dgdavid changed the title ~~Feature/service widget~~ Use the new ServiceWidget Aug 1, 2018

teclator reviewed Aug 1, 2018

View reviewed changes

dgdavid force-pushed the feature/service_widget branch from cc96351 to 90fc0fb Compare August 1, 2018 15:35

joseivanlopez reviewed Aug 7, 2018

View reviewed changes

dgdavid force-pushed the feature/service_widget branch from f0374a0 to a46872d Compare August 7, 2018 10:17

joseivanlopez reviewed Aug 7, 2018

View reviewed changes

dgdavid force-pushed the feature/service_widget branch from 8f68cfb to e2506f8 Compare August 8, 2018 15:52

dgdavid added 3 commits August 10, 2018 09:38

Use CWM::ServiceWidget to handle the service start up configuration

ec8ce14

Use CWM::ServiceWidget also in the wizard installation (first run)

50a74a5

dgdavid force-pushed the feature/service_widget branch from e2506f8 to aa02e8b Compare August 10, 2018 08:40

dgdavid changed the base branch from SLE-15-GA to master August 10, 2018 08:41

dgdavid force-pushed the feature/service_widget branch from aa02e8b to ee9e8ac Compare August 10, 2018 08:42

dgdavid added 4 commits August 10, 2018 10:09

Move duplicated logic to helper module

f58b0d1

Much of the methods related to status service widget were duplicated in both dialog, reason why it has been moved to a helper module.

Update Yast2 required version

2b05d1e

Update from code review

f99c0fb

Bump version and changes file

67c97a6

dgdavid force-pushed the feature/service_widget branch from ee9e8ac to 67c97a6 Compare August 10, 2018 09:09

Update unit tests

b029053

imobachgs reviewed Aug 10, 2018

View reviewed changes

Update from code review

fb6a305

dgdavid force-pushed the feature/service_widget branch from c90effd to fb6a305 Compare August 10, 2018 11:31

imobachgs approved these changes Aug 10, 2018

View reviewed changes

dgdavid merged commit b6c8b24 into master Aug 10, 2018

dgdavid deleted the feature/service_widget branch August 10, 2018 14:22

dgdavid mentioned this pull request Aug 14, 2018

Moves the manage of start mode and service status from Perl to Ruby #74

Merged

teclator mentioned this pull request Sep 20, 2019

Fix wizard service save (bsc#1123985) #84

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the new ServiceWidget #73

Use the new ServiceWidget #73

dgdavid commented Aug 1, 2018 •

edited

Loading

teclator Aug 1, 2018

teclator commented Aug 1, 2018

dgdavid commented Aug 1, 2018

joseivanlopez Aug 7, 2018

joseivanlopez Aug 7, 2018

dgdavid Aug 7, 2018

joseivanlopez Aug 7, 2018

dgdavid Aug 7, 2018 •

edited

Loading

joseivanlopez Aug 7, 2018

joseivanlopez Aug 7, 2018

dgdavid Aug 7, 2018

joseivanlopez Aug 7, 2018

imobachgs Aug 10, 2018

joseivanlopez Aug 7, 2018

joseivanlopez Aug 7, 2018

joseivanlopez Aug 7, 2018

joseivanlopez Aug 7, 2018

joseivanlopez Aug 7, 2018

dgdavid Aug 7, 2018

joseivanlopez Aug 7, 2018

dgdavid Aug 7, 2018

joseivanlopez left a comment

coveralls commented Aug 10, 2018 •

edited

Loading

imobachgs left a comment

imobachgs Aug 10, 2018

imobachgs Aug 10, 2018

imobachgs Aug 10, 2018

dgdavid Aug 10, 2018

imobachgs Aug 10, 2018

imobachgs Aug 10, 2018

imobachgs Aug 10, 2018

imobachgs Aug 10, 2018

dgdavid Aug 10, 2018

imobachgs Aug 10, 2018

Use the new ServiceWidget #73

Use the new ServiceWidget #73

Conversation

dgdavid commented Aug 1, 2018 • edited Loading

Choose a reason for hiding this comment

teclator commented Aug 1, 2018

dgdavid commented Aug 1, 2018

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

dgdavid Aug 7, 2018 • edited Loading

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

joseivanlopez left a comment

Choose a reason for hiding this comment

coveralls commented Aug 10, 2018 • edited Loading

imobachgs left a 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

dgdavid commented Aug 1, 2018 •

edited

Loading

dgdavid Aug 7, 2018 •

edited

Loading

coveralls commented Aug 10, 2018 •

edited

Loading