Skip to content

Unit_file: Add notify_service #295

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Unit_file: Add notify_service #295

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions manifests/unit_file.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@
# @param daemon_reload
# call `systemd::daemon-reload` to ensure that the modified unit file is loaded
#
# @param notify_service
# Notify service if systemd file is changed
#
# @example manage unit file + service
# systemd::unit_file { 'foo.service':
# content => file("${module_name}/foo.service"),
Expand All @@ -88,7 +91,8 @@
Optional[Boolean] $hasstatus = undef,
Boolean $selinux_ignore_defaults = false,
Hash[String[1], Any] $service_parameters = {},
Boolean $daemon_reload = true
Boolean $daemon_reload = true,
Boolean $notify_service = true,
) {
include systemd

Expand Down Expand Up @@ -152,7 +156,11 @@
}
Service[$name] -> File["${path}/${name}"]
} else {
File["${path}/${name}"] ~> Service[$name]
if $notify_service {
File["${path}/${name}"] ~> Service[$name]
} else {
File["${path}/${name}"] -> Service[$name]
}

if $daemon_reload {
Systemd::Daemon_reload[$name] ~> Service[$name]
Expand Down
10 changes: 10 additions & 0 deletions spec/defines/unit_file_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,16 @@
expect(subject).not_to create_systemd__daemon_reload(title)
}
end

context 'notify_service => false' do
let(:params) { { enable: true, notify_service: false } }

it do
is_expected.not_to contain_service(title).with_subscribes_to(
Copy link
Member

Choose a reason for hiding this comment

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

with_subscribes_to expects a property to be set, like

service { $title:
  subscribes_to => File[$file],
}

That's never going to be true. This checks the compiled catalog (which mirrors the check on line 210 for example):

Suggested change
is_expected.not_to contain_service(title).with_subscribes_to(
is_expected.not_to contain_service(title).that_subscribes_to(

Copy link
Author

Choose a reason for hiding this comment

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

That not work, but something like this look ok:

           let(:params) { { enable: true, notify_service: false } }
 
           it do
-            is_expected.not_to contain_service(title).with_subscribes_to(
-              "File[/etc/systemd/system/#{title}]"
+            is_expected.not_to contain_file("/etc/systemd/system/#{title}").with(
+              notify: /Service\[#{title}\]/
             )
           end
         end

It's good for you ?

Copy link
Member

Choose a reason for hiding this comment

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

No, that's not the same. Please see https://rspec-puppet.com/documentation/classes/#testing-relationships-between-resources for the background. Where with doesn't doesn't take ~> into account, that_... does.

Your suggestion would fail on this:

file { "/etc/systemd/system/${title}":
  notify => Service[$title],
}

It would not fail on:

file { "/etc/systemd/system/${title}":
}

File["/etc/systemd/system/${title}"] ~> Service[$title]

The reason it now fails is a transitive property. See rodjek/rspec-puppet#821 for more discussion, but there's a chain:

File["/etc/systemd/system/${name}"] ~> Systemd::Daemon_reload[$name] ~> Service[$name]

rspec-puppet thinks this implies:

File["/etc/systemd/system/${name}"] ~> Service[$name]

https://puppet.com/docs/puppet/7/lang_relationships.html#lang_rel_chaining_arrows is unclear about this. However, now in practice it probably is. Because the daemon reload is notified, that probably triggers a service restart anyway. So if $daemon_reload is true (default value), I don't think your patch is valid.

Copy link
Author

Choose a reason for hiding this comment

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

I've try before, with my path when it's true that fail correctly:

diff --git a/spec/defines/unit_file_spec.rb b/spec/defines/unit_file_spec.rb
index a0521aa..6dde40a 100644
--- a/spec/defines/unit_file_spec.rb
+++ b/spec/defines/unit_file_spec.rb
@@ -272,8 +272,18 @@ describe 'systemd::unit_file' do
           let(:params) { { enable: true, notify_service: false } }
 
           it do
-            is_expected.not_to contain_service(title).with_subscribes_to(
-              "File[/etc/systemd/system/#{title}]"
+            is_expected.not_to contain_file("/etc/systemd/system/#{title}").with(
+              notify: /Service\[#{title}\]/
+            )
+          end
+        end
+
+        context 'notify_service => true' do
+          let(:params) { { enable: true, notify_service: true } }
+
+          it do
+            is_expected.not_to contain_file("/etc/systemd/system/#{title}").with(
+              notify: /Service\[#{title}\]/
             )
           end
         end
...
  21) systemd::unit_file supported operating systems on centos-8-x86_64 notify_service => true is expected not to contain File[/etc/systemd/system/test.service] with notify =~ /Service\[test.service\]/
      Failure/Error:
        is_expected.not_to contain_file("/etc/systemd/system/#{title}").with(
          notify: /Service\[#{title}\]/
        )
      
        expected that the catalogue would not contain File[/etc/systemd/system/test.service]
      # ./spec/defines/unit_file_spec.rb:285:in `block (6 levels) in <top (required)>'

Finished in 1 minute 49.83 seconds (files took 54.25 seconds to load)
609 examples, 21 failures
...

Copy link
Author

Choose a reason for hiding this comment

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

Hello,

Any update/idea about this test ?

Copy link
Member

Choose a reason for hiding this comment

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

I still don't know what the proper behavior is: puppetlabs/rspec-puppet#35 is still open. It may well be that with your current implementation it's impossible to prevent a notification to propagate to the service.

"File[/etc/systemd/system/#{title}]"
)
end
end
end
end
end
Expand Down