Skip to content
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

systemd::unit_file: remove hasrestart/hasstatus params #264

Merged
merged 2 commits into from
Jan 26, 2023
Merged

systemd::unit_file: remove hasrestart/hasstatus params #264

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
76577c3
systemd::unit_file: remove hasrestart/hasstatus params
bastelfreak Mar 2, 2022
d0e41a3
Merge branch 'master' into removeparams
jhoblitt Jan 26, 2023
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
18 changes: 0 additions & 18 deletions REFERENCE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1223,8 +1223,6 @@ The following parameters are available in the `systemd::unit_file` defined type:
* [`enable`](#enable)
* [`active`](#active)
* [`restart`](#restart)
* [`hasrestart`](#hasrestart)
* [`hasstatus`](#hasstatus)
* [`selinux_ignore_defaults`](#selinux_ignore_defaults)
* [`service_parameters`](#service_parameters)

Expand Down Expand Up @@ -1336,22 +1334,6 @@ Specify a restart command manually. If left unspecified, a standard Puppet servi

Default value: ``undef``

##### <a name="hasrestart"></a>`hasrestart`

Data type: `Optional[Boolean]`

maps to the same param on the service resource. Optional in the module because it's optional in the service resource type. This param is deprecated. Set it via $service_parameters.

Default value: ``undef``

##### <a name="hasstatus"></a>`hasstatus`

Data type: `Optional[Boolean]`

maps to the same param on the service resource. true in the module because it's true in the service resource type. This param is deprecated. Set it via $service_parameters.

Default value: ``undef``

##### <a name="selinux_ignore_defaults"></a>`selinux_ignore_defaults`

Data type: `Boolean`
Expand Down
28 changes: 5 additions & 23 deletions manifests/unit_file.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,6 @@
# @param restart
# Specify a restart command manually. If left unspecified, a standard Puppet service restart happens.
#
# @param hasrestart
# maps to the same param on the service resource. Optional in the module because it's optional in the service resource type. This param is deprecated. Set it via $service_parameters.
#
# @param hasstatus
# maps to the same param on the service resource. true in the module because it's true in the service resource type. This param is deprecated. Set it via $service_parameters.
#
# @param selinux_ignore_defaults
# maps to the same param on the file resource for the unit. false in the module because it's false in the file resource type
#
Expand All @@ -81,23 +75,13 @@
Optional[Variant[Boolean, Enum['mask']]] $enable = undef,
Optional[Boolean] $active = undef,
Optional[String] $restart = undef,
Copy link
Contributor

Choose a reason for hiding this comment

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

Shoudn't the restart parameter be removed with this as well?

Copy link
Member

Choose a reason for hiding this comment

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

This is the restart parameter on service, which I think can still be useful. For example, one could set /usr/bin/systemctl reload-or-restart $unit.

If you're talking about the one in service_limits, then I'd agree. IMHO that's a bad pattern.

puppet-systemd/manifests/service_limits.pp

Line 38 in d9d99c3

Boolean $restart_service = true

Copy link
Contributor

Choose a reason for hiding this comment

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

systemd::unit_file { "my.service":
  ....
  service_parameters => {
    'restart' => "/usr/bin/systemctl reload-or-restart $unit"
  }
}

is enough. .. Or is intention to just not add every single option that could possibly go to service - also makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

Oh that's an excellent point. I think overriding this is obscure enough that it's valid to not have a dedicated parameter. 👍

Optional[Boolean] $hasrestart = undef,
Optional[Boolean] $hasstatus = undef,
Boolean $selinux_ignore_defaults = false,
Hash[String[1], Any] $service_parameters = {},
) {
include systemd

assert_type(Systemd::Unit, $name)

if $hasrestart =~ NotUndef {
deprecation("systemd::unit_file - ${title}", 'hasrestart is deprecated and will be removed in Version 4 of the module')
}

if $hasstatus =~ NotUndef {
deprecation("systemd::unit_file - ${title}", 'hasstatus is deprecated and will be removed in Version 4 of the module')
}

if $enable == 'mask' {
$_target = '/dev/null'
} else {
Expand Down Expand Up @@ -127,13 +111,11 @@

if $enable != undef or $active != undef {
service { $name:
ensure => $active,
enable => $enable,
restart => $restart,
provider => 'systemd',
hasrestart => $hasrestart,
hasstatus => $hasstatus,
* => $service_parameters,
ensure => $active,
enable => $enable,
restart => $restart,
provider => 'systemd',
* => $service_parameters,
}

if $ensure == 'absent' {
Expand Down
83 changes: 0 additions & 83 deletions spec/defines/unit_file_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,89 +122,6 @@
end
end

context 'with enable => true and active => true and service_parameters and duplicate param' do
let(:params) do
super().merge(
enable: true,
active: true,
hasrestart: true,
service_parameters: {
flags: '--awesome',
timeout: 1337,
hasrestart: false
}
)
end

it { is_expected.not_to compile }
end

context 'hasrestart => true' do
let(:params) do
super().merge(
enable: true,
active: true,
hasrestart: true
)
end

it { is_expected.to compile.with_all_deps }

it do
expect(subject).to contain_service('test.service').
with_ensure(true).
with_enable(true).
with_provider('systemd').
without_hasstatus.
with_hasrestart(true).
that_subscribes_to("File[/etc/systemd/system/#{title}]")
end
end

context 'hasrestart => false' do
let(:params) do
super().merge(
enable: true,
active: true,
hasrestart: false
)
end

it { is_expected.to compile.with_all_deps }

it do
expect(subject).to contain_service('test.service').
with_ensure(true).
with_enable(true).
with_provider('systemd').
without_hasstatus.
with_hasrestart(false).
that_subscribes_to("File[/etc/systemd/system/#{title}]")
end
end

context 'hasstatus => false' do
let(:params) do
super().merge(
enable: true,
active: true,
hasstatus: false
)
end

it { is_expected.to compile.with_all_deps }

it do
expect(subject).to contain_service('test.service').
with_ensure(true).
with_enable(true).
with_provider('systemd').
with_hasstatus(false).
without_hasrestart.
that_subscribes_to("File[/etc/systemd/system/#{title}]")
end
end

context 'ensure => absent' do
let(:params) { super().merge(ensure: 'absent') }

Expand Down