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

update supported OSes in params.pp #296

Merged
merged 2 commits into from
Sep 27, 2018

Conversation

Dan33l
Copy link
Member

@Dan33l Dan33l commented Sep 14, 2018

Pull Request (PR) description

update params.pp to be up to date with current metadata.json

This Pull Request (PR) fixes the following issues

Fixes #291

@Dan33l Dan33l force-pushed the update-supported-os-params-pp branch from bd0ed8c to 27313c7 Compare September 14, 2018 20:42
@Dan33l Dan33l changed the title update supported OSes in params.pp WIP : update supported OSes in params.pp Sep 14, 2018
@Dan33l Dan33l force-pushed the update-supported-os-params-pp branch from 27313c7 to 91382fb Compare September 14, 2018 21:30
@@ -16,98 +16,71 @@
#
class openvpn::params {

case $::osfamily {
'RedHat': {
case $facts['os']['family'] {
Copy link
Member

Choose a reason for hiding this comment

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

structured facts \o/

'6': {
$additional_packages = ['easy-rsa','openvpn-auth-ldap']
$ldap_auth_plugin_location = '/usr/lib64/openvpn/plugin/lib/openvpn-auth-ldap.so'
$systemd = false
Copy link
Member

Choose a reason for hiding this comment

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

suggestion for future enhancements: $systemd can be set to true if the fact [service_provider](https://github.com/puppetlabs/puppetlabs-stdlib#service_provider) returns systemd

osfamily: 'Debian',
operatingsystem: 'Debian',
operatingsystemrelease: '7',
os: { 'family' => 'Debian' },
Copy link
Member

Choose a reason for hiding this comment

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

this should be migrated to rspec-puppet-facts, same applies for the other spec files.

Copy link
Member Author

@Dan33l Dan33l Sep 21, 2018

Choose a reason for hiding this comment

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

You're right, but the tag needs-work was not useless.
I did it.

@bastelfreak bastelfreak added enhancement New feature or request needs-work not ready to merge just yet labels Sep 14, 2018
@Dan33l Dan33l force-pushed the update-supported-os-params-pp branch 3 times, most recently from 68fbe0d to 1057c9d Compare September 21, 2018 14:41
@Dan33l Dan33l closed this Sep 21, 2018
@Dan33l Dan33l reopened this Sep 21, 2018
@Dan33l Dan33l removed the needs-work not ready to merge just yet label Sep 21, 2018
@Dan33l Dan33l changed the title WIP : update supported OSes in params.pp update supported OSes in params.pp Sep 21, 2018
context "on #{os}" do
let(:facts) do
facts.merge(
concat_basedir: '/var/lib/puppet/concat'
Copy link
Member

Choose a reason for hiding this comment

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

Did you test if this is required? We have mocked facts here:

concat_basedir: "/tmp"

And use them here:

RSpec.configure do |c|
default_facts = {
puppetversion: Puppet.version,
facterversion: Facter.version
}
default_facts.merge!(YAML.load(File.read(File.expand_path('../default_facts.yml', __FILE__)))) if File.exist?(File.expand_path('../default_facts.yml', __FILE__))
default_facts.merge!(YAML.load(File.read(File.expand_path('../default_module_facts.yml', __FILE__)))) if File.exist?(File.expand_path('../default_module_facts.yml', __FILE__))
c.default_facts = default_facts
c.mock_with :rspec
c.hiera_config = 'spec/fixtures/hiera/hiera.yaml'
end

@bastelfreak
Copy link
Member

If I see this correctly this PR doesnt add support for new operating systems, right? Otherwise the list in the metadata.json needs to be updated as well.

@Dan33l Dan33l force-pushed the update-supported-os-params-pp branch from 1057c9d to 9dd0d19 Compare September 22, 2018 18:45
@bastelfreak
Copy link
Member

I created a little patch that you might want to look into:

diff --git a/spec/classes/openvpn_config_spec.rb b/spec/classes/openvpn_config_spec.rb
index 10b4e73..d33e35d 100644
--- a/spec/classes/openvpn_config_spec.rb
+++ b/spec/classes/openvpn_config_spec.rb
@@ -4,12 +4,12 @@ describe 'openvpn::config', type: :class do
   on_supported_os.each do |os, facts|
     context "on #{os}" do
       let(:facts) do
-        facts.merge(
-          concat_basedir: '/var/lib/puppet/concat'
-        )
+        facts
       end
 
-      case facts[:os][:family]
+      it { is_expected.to compile.with_all_deps }
+
+      case facts[:os]['family']
       when 'Debian'
         context 'on Debian based machines' do
           it { is_expected.to contain_concat('/etc/default/openvpn') }

A couple of things:

  • I tested this, we don't need to mock concat_basedir
  • in the facts hash, only the first key is a symbol, all further nested keys are strings
  • It's always good to check if the manifest compiles into a catalog

@Dan33l Dan33l force-pushed the update-supported-os-params-pp branch 2 times, most recently from ea78ae5 to 67f057a Compare September 26, 2018 16:38
@Dan33l Dan33l force-pushed the update-supported-os-params-pp branch from 67e5130 to cae1372 Compare September 26, 2018 19:26
@Dan33l
Copy link
Member Author

Dan33l commented Sep 27, 2018

If I see this correctly this PR doesnt add support for new operating systems, right? Otherwise the list in the metadata.json needs to be updated as well.

Exactly . No new OS.

@@ -454,7 +454,7 @@
Optional[String] $group = undef,
Boolean $ipp = false,
Boolean $duplicate_cn = false,
String $local = $::ipaddress_eth0,
String $local = $facts['ipaddress_eth0'],
Copy link
Member

Choose a reason for hiding this comment

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

facts hash \o/

@bastelfreak
Copy link
Member

thanks for this awesome PR!

@bastelfreak bastelfreak merged commit 0f2c127 into voxpupuli:master Sep 27, 2018
@Dan33l Dan33l deleted the update-supported-os-params-pp branch September 27, 2018 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

debian 7 support broken
2 participants