Move some systemd modules to real classes #758

dgdavid · 2018-07-04T11:46:16Z

Listed "class modules" below has been moved to their owns real classes

SystemdServiceClass -> SystemdService
SystemdSocketClass -> SystemdSocket
SystemdTargetClass -> SystemdTarget

joseivanlopez · 2018-07-04T13:27:27Z

library/systemd/src/lib/yast2/systemd_service.rb

@@ -0,0 +1,187 @@
+require "yast2/systemd_unit"
+
+module Yast


I guess we should start to use Yast2 namespace for this class.

joseivanlopez · 2018-07-04T15:40:52Z

library/system/src/lib/yast2/system_service.rb

    def initialize(service)
      @service = service
    end

    # Returns socket associated with service or nil if there is no such socket
    #
-    # @return [Yast::SystemdSocketClass::Socket]
+    # @return [Yast::Systemd::Socket]


Yast2::Systemd::Socket

joseivanlopez · 2018-07-04T15:41:36Z

library/system/src/lib/yast2/system_service.rb

@@ -31,7 +31,7 @@ module Yast2
  class SystemService
    extend Forwardable

-    # @return [Yast::SystemdService]
+    # @return [Yast2::Systemd::Service]


Not necessary Yast2 here because we are already under Yast2 namespace.

joseivanlopez · 2018-07-04T15:42:47Z

library/system/src/lib/yast2/system_service.rb

      end
    end

-    # @param service [Yast::SystemdServiceClass::Service]
+    # @param service [Yast2::Systemd::Service]


Not needed Yast2.

joseivanlopez · 2018-07-04T15:43:31Z

library/system/src/lib/yast2/system_service.rb

@@ -76,7 +76,7 @@ def socket
      socket_name = socket_name[/\S+\.socket/]
      return unless socket_name # triggered by non-socket

-      @socket = Yast::SystemdSocket.find(socket_name)
+      @socket = Yast::Systemd::Socket.find(socket_name)


Does it work? It should be @socket = Systemd::Socket.find(socket_name

Oops 💩! Good catch!

joseivanlopez · 2018-07-04T15:52:06Z

library/systemd/src/lib/yast2/systemd_service.rb

+require "yast2/systemd_unit"
+
+module Yast2
+  module Systemd


To be compliant with ruby standards, the name space hierarchy should match the directories structure, so this file should be placed in yast2/systemd/service.rb. I would also create a new file yast2/systemd.rb with a content as follows:

require "yast2/systemd/unit" require "yast2/systemd/target" require "yast2/systemd/service" require "yast2/systemd/socket" module Systemd; end

joseivanlopez · 2018-07-04T15:56:54Z

library/systemd/src/lib/yast2/systemd_socket.rb

+require "yast2/systemd_unit"
+
+module Yast2
+  module Systemd


Same comment as before, it should be placed at yast2/systemd/socket.rb.

joseivanlopez · 2018-07-04T15:57:41Z

library/systemd/src/lib/yast2/systemd_target.rb

+require "yast2/systemd_unit"
+
+module Yast2
+  module Systemd


And again the same, yast2/systemd/target.rb

joseivanlopez · 2018-07-04T16:00:40Z

library/systemd/src/lib/yast2/systemd_unit.rb

@@ -2,309 +2,311 @@



and here the last one. Please, move it to yast2/systemd/unit.rb

joseivanlopez · 2018-07-04T16:06:21Z

library/systemd/test/test_helper.rb

@@ -23,7 +22,7 @@ def stub_systemctl(unit)
  end

  def stub_execute(success: true)
-    allow(Yast::Systemctl).to receive(:execute).and_return(
+    allow(Yast2::Systemctl).to receive(:execute).and_return(


Reading this I have the doubt if Systemctl should also be under Systemd namespace. @jreidinger @imobachgs ?

joseivanlopez · 2018-07-04T16:11:53Z

library/systemd/test/yast2/systemd_service_test.rb

@@ -48,36 +46,36 @@ module Yast
      let(:apparmor_double) { double("Service", name: "apparmor") }
      let(:cups_double) { double("Service", name: "cups") }
      let(:systemctl_stdout) do
-        File.read(File.join(__dir__, "data", "apparmor_and_cups_properties"))
+        File.read(File.join(File.expand_path("..", __dir__), "data", "apparmor_and_cups_properties"))


For this I think it is better to define a data path in test_helper.rb and then use it here.

In test_helper.rb:
DATA_PATH = File.expand_path("../data", __FILE__)

In test:
File.read(File.join(DATA_PATH, "apparmor_and_cups_properties")

joseivanlopez · 2018-07-04T16:15:48Z

library/systemd/test/yast2/systemd_target_test.rb

@@ -63,70 +61,70 @@ module Yast

    describe ".get_default" do
      it "returns the unit object of the currently set default target" do
-        allow(Systemctl).to receive(:execute).with("get-default").and_return(
+        allow(Yast2::Systemctl).to receive(:execute).with("get-default").and_return(


NP: not needed Yast2

joseivanlopez · 2018-07-04T16:16:01Z

library/systemd/test/yast2/systemd_target_test.rb

        expect(target.unit_name).to eq("graphical")
      end
    end

    describe ".set_default" do
      it "returns true if the default target has been has for the parameter successfully" do
-        expect(Systemctl).to receive(:execute).with("set-default --force graphical.target")
+        expect(Yast2::Systemctl).to receive(:execute).with("set-default --force graphical.target")


NP: not needed Yast2

joseivanlopez · 2018-07-04T16:16:23Z

library/systemd/test/yast2/systemd_target_test.rb

      end
    end

    describe "#set_default" do
      it "it returns true if the target unit object has been set as default target" do
-        expect(Systemctl).to receive(:execute).with("set-default --force graphical.target")
+        expect(Yast2::Systemctl).to receive(:execute).with("set-default --force graphical.target")


NP: not needed Yast2

joseivanlopez · 2018-07-04T16:16:37Z

library/systemd/test/yast2/systemd_target_test.rb

        expect(target.set_default).to eq(false)
      end

      context "when target properties cannot be found out (e.g. in chroot)" do
        it "it returns true if the target unit object has been set as default target" do
-          expect(Systemctl).to receive(:execute).with("set-default --force multi-user-in-installation.target")
+          expect(Yast2::Systemctl).to receive(:execute).with("set-default --force multi-user-in-installation.target")


NP: not needed Yast2

coveralls · 2018-07-06T13:14:04Z

Coverage decreased (-0.01%) to 29.268% when pulling 5aa2e71 on dgdavid:feature/move-module-to-real-classes into 6be7ec7 on yast:socket_feature_branch.

- SystemService#active_state - SystemService#sub_state

* Relocate systemd classes to `lib/yast2/systemd/`, matching directories with with name space hierarchy * Fix wrong `Yast::` prefix * Remove not necessaries `Yast2::` prefixes * Improve data path in test

dgdavid · 2018-08-14T16:04:57Z

This PR is too outdated, so I am closing it in favor of a new one.

See #799

joseivanlopez reviewed Jul 4, 2018

View reviewed changes

jreidinger and others added 23 commits August 14, 2018 09:52

add API to connect service and socket

a8b8141

add missing test data

c6ccb5b

fix typo

29154b9

Fix find_many_at_once to retrieve service properties

49d445e

Improve SystemdService.find_many testing

e5d0e4d

use active and not running for services (bsc#1080738#c3)

d960ad2

update documentation and fix tests

ade6109

add initial RFC for service configuration

6c475c5

and add RFC for service widget

d63dbd5

implementation WIP

94d9498

Extend SystemdService API

0ea2fa6

Memoize SystemdService#socket

143c515

Updates from code review

9d648f9

Updates from code review

860ff06

Improve enable/disable tests

f5c1206

Keep old enable/disable SystemdService API

d009278

remove reload flag and add reload state and implement some methods

7f90753

Rename :boot and :demand to :on_boot and :on_demand

c7966bf

Add Yast2::SystemService class

5001601

document new start behavior

6c009ca

add working examples

7186eac

code review

3859012

Delegate some SystemService to Yast::SystemdServiceClass::Service

747e2de

imobachgs and others added 25 commits August 14, 2018 09:52

Add system_service.rb

b13a3a7

Add minimal documentation to Yast2::SystemService

63c72fc

Document and test SystemdService.find

3886441

Add a SystemdService.find_many finder

00df561

Add SystemService test to Makefile.am

2365395

Simplify SystemService#start_modes

dd02a47

make rubocop happy

f914ba4

Fix SystemService documentation

b9c3eb4

Add test for configuration

74995ff

fix rubocop

e522280

try to fix old makefile

0e0e9e1

Add methods to read the service state

ce36d36

- SystemService#active_state - SystemService#sub_state

Add missing ServiceConfiguration to makefiles

98bebfe

Adapt SystemService to keep changes in memory

d734ab3

Move SystemService#socket method to SystemdService class

37d7908

Add more agnostic method names

53ef7cf

Add method SystemService#search_terms

dc10b66

Fix text

4141edf

Move SystemdServiceClass module to real SystemdService class

bef5e73

Move SystemdSocketClass module to real SystemdSocket class

82bf16d

Move SystemdTargetClass module to real SystemdTarget class

1ed5d03

Move systemd classes to Yast2 namespace

2285150

Update Makefile.am

341464a

Fixes from code review

6ab6436

* Relocate systemd classes to `lib/yast2/systemd/`, matching directories with with name space hierarchy * Fix wrong `Yast::` prefix * Remove not necessaries `Yast2::` prefixes * Improve data path in test

Update systemd Makefiles for test

dc5f2fa

dgdavid changed the base branch from socket_feature_branch to master August 14, 2018 08:53

dgdavid changed the base branch from master to SLE-15-GA August 14, 2018 09:37

dgdavid closed this Aug 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move some systemd modules to real classes #758

Move some systemd modules to real classes #758

dgdavid commented Jul 4, 2018

joseivanlopez Jul 4, 2018

joseivanlopez Jul 4, 2018

joseivanlopez Jul 4, 2018

joseivanlopez Jul 4, 2018

joseivanlopez Jul 4, 2018

dgdavid Jul 4, 2018

joseivanlopez Jul 4, 2018 •

edited

joseivanlopez Jul 4, 2018

joseivanlopez Jul 4, 2018

joseivanlopez Jul 4, 2018

joseivanlopez Jul 4, 2018

joseivanlopez Jul 4, 2018

joseivanlopez Jul 4, 2018

joseivanlopez Jul 4, 2018

joseivanlopez Jul 4, 2018

joseivanlopez Jul 4, 2018

coveralls commented Jul 6, 2018

dgdavid commented Aug 14, 2018

Move some systemd modules to real classes #758

Move some systemd modules to real classes #758

Conversation

dgdavid commented Jul 4, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joseivanlopez Jul 4, 2018 • edited

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 6, 2018

dgdavid commented Aug 14, 2018

joseivanlopez Jul 4, 2018 •

edited