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.

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

Feature/master/systemd compatibility #90

Closed

Conversation

bjeske
Copy link

@bjeske bjeske commented Aug 22, 2016

Ensure => 'absent' for initd softlink on systems with systemd like ubuntu 16.04 etc.

@@ -31,6 +31,12 @@
$service_enable = true
}

if ($::operatingsystem == 'Ubuntu' and $::operatingsystemrelease in ['15.04','15.10','16.04','16.10']) or ($::operatingsystem == 'Debian' and $::operatingsystemrelease == '8') {
Copy link
Member

Choose a reason for hiding this comment

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

It might be better to simplify this to use the systemd fact:

https://github.com/camptocamp/puppet-systemd/blob/master/lib/facter/systemd.rb

This would mean this patch would also work on RHEL flavoured systems with systemd (Centos 7, Oracle Linux 7, Fedora 14+ etc)

Copy link
Author

Choose a reason for hiding this comment

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

This is not a core fact, right? So we would need a dependency for puppet-systemd... I'm not so sure if this is a good decision? As of Ubuntu 16.04 with facter 1.7.5 this fact is not present.

Copy link
Member

Choose a reason for hiding this comment

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

It's not a core fact unfortunately no. Adding a dependency is one option, but seems a bit heavy handed for just one fact.

However, it could be added as a fact to this module with it's own namespace (eg. gitlab_systemd_check or something), using only the code for the systemd check from the linked fact:

Facter.add(:gilab_systemd_check) do
  confine :kernel => :linux
  setcode do
    Facter::Core::Execution.exec('ps -p 1 -o comm=') == 'systemd'
  end
end

The benefit of adding the fact would be that the code becomes a lot more manageable:

if ($::gilab_systemd_check) {

Copy link
Author

Choose a reason for hiding this comment

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

It seems "Facter::Core::Execution" is not present in facter 1.7.5, which comes with Ubuntu 16.04.
I had to use the "old %x" Sysntax to make this work:

Facter.add(:gitlab_systemd) do
  confine :kernel => :linux
  setcode do
    %x{ps -p 1 -o comm=}.gsub("\n",'') == 'systemd'
  end
end

Or did i miss something here?

Copy link
Member

Choose a reason for hiding this comment

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

Using backticks/%x or Kernel.exec will ignore the confine, so it will run on any system rather than be restricted to Linux (eg. Windows, OSX, BSD).

Good spot, It should be Facter::Util::Resolution.exec which is backwards compatible:
http://www.rubydoc.info/gems/facter/1.7.5/Facter/Util/Resolution.exec
http://www.rubydoc.info/gems/facter/2.4.6/Facter/Util/Resolution.exec

Facter.add(:gitlab_systemd) do
  confine :kernel => :linux
  setcode do
     Facter::Util::Resolution.exec('ps -p 1 -o comm=') == 'systemd'
  end

end

I'll open a PR against the systemd repo for that to fix that! 👍

Copy link
Author

Choose a reason for hiding this comment

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

Seems like i was a bit fast claiming 16.04 comes with 1.7.5... Not so sure if we enforce this facter version for one reason or another somewhere... 2.4.6 seems to be in 16.04.

Copy link
Member

Choose a reason for hiding this comment

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

Heh, I think it depends if you're using Ubuntu upstreams or Puppet's ones 😄

Either way, Util resolution is the right way 👍

@@ -28,6 +28,12 @@
# Default: true
# Run the system service on boot.
#
# [*service_initd_ensure*]
# Default for 'Ubuntu 15.04, 15.10, 16.04, 16.10' or 'Debian 8': "absent"
Copy link
Member

Choose a reason for hiding this comment

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

This probably needs changing to "default for systemd systems, as determined by the $:: gitlab_systemd fact

@tobru
Copy link
Contributor

tobru commented Sep 16, 2016

@bjeske Thanks a lot for your contribution. Before I can merge it could you please make sure that all tests passes? And if you could do a rebase so we only have one commit to merge would also be great.

@petems petems mentioned this pull request Sep 16, 2016
@petems
Copy link
Member

petems commented Sep 16, 2016

@tobru I've rebased, squashed and added tests for this PR in #94, as I'm looking to move to the upstream for a customer rather than a fork.

@bjeske The commit is still under your name in the new PR, but squashed and tests fixed

@tobru
Copy link
Contributor

tobru commented Sep 19, 2016

Merged in #94

@tobru tobru closed this Sep 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants