Data in Modules, Modern facts & Cleanup #305

jkroepke · 2018-10-20T22:15:43Z

PR is still in progress.

Reviews are welcome.

Pull Request (PR) description

Add Data in modules
Add defaults for unsupported OS
Used modern facts
Removed top scope variables
Removed create_resources
Removed params.pp
Remove systemd option in favor of facts provided by stdlib

This Pull Request (PR) fixes the following issues

Fixes #304

Tested

Ubuntu 18.04 (no fresh installation, no changes)

bastelfreak · 2018-10-21T06:42:10Z

Dockerfile

@@ -0,0 +1,17 @@
+FROM ruby:2.4


I'm not sure if we want a Dockerfile. If so, we should probably contribute it to https://github.com/voxpupuli/modulesync_config/tree/master/moduleroot

bastelfreak · 2018-10-21T06:44:40Z

manifests/deploy/prepare.pp

-
-  class { 'openvpn::params': }
+class openvpn::deploy::prepare(
+  String $etc_directory,


We can validate this even stricter:

Suggested change

String $etc_directory,

Stdlib::Absolutepath $etc_directory,

bastelfreak · 2018-10-21T06:45:32Z

manifests/init.pp

-  Hash $servers                         = {},
+  Boolean                    $autostart_all,
+  Boolean                    $manage_service,
+  String                     $etc_directory,


Suggested change

String $etc_directory,

Stdlib::Absolutepath $etc_directory,

bastelfreak · 2018-10-21T06:45:43Z

manifests/init.pp

+  Boolean                    $autostart_all,
+  Boolean                    $manage_service,
+  String                     $etc_directory,
+  String                     $group,


Suggested change

String $group,

String[1] $group,

bastelfreak · 2018-10-21T06:46:35Z

manifests/init.pp

+  Boolean                    $namespecific_rclink,
+  Pattern[/^[23]\.0$/]       $default_easyrsa_ver,
+  Stdlib::Unixpath           $easyrsa_source,
+  Variant[String, Array]     $additional_packages,


Suggested change

Variant[String, Array] $additional_packages,

Variant[String[1], Array[String[1]]] $additional_packages,

bastelfreak · 2018-10-21T06:47:30Z

manifests/install.pp


  ensure_packages(['openvpn'])
-  if $::openvpn::params::additional_packages != undef {
-    ensure_packages( any2array($::openvpn::params::additional_packages) )
+  if $openvpn::additional_packages != undef {


This could be simplified to:

Suggested change

if $openvpn::additional_packages != undef {

if $openvpn::additional_packages {

bastelfreak · 2018-10-21T06:51:31Z

data/defaults.yaml

+openvpn::easyrsa_source: '/usr/share/easy-rsa/'
+openvpn::additional_packages: ['easy-rsa']
+openvpn::ldap_auth_plugin_location: ~
+openvpn::systemd: false


Do we need this parameter?

$ git grep systemd CHANGELOG.md:* Add systemd support for Debian >= 8.0 ([#161](https://github.com/luxflux/puppet-openvpn/pull/161)) CHANGELOG.md:* Support for systemd ([#127](https://github.com/luxflux/puppet-openvpn/pull/127)) HISTORY.md:* Add systemd support for Debian >= 8.0 ([#161](https://github.com/luxflux/puppet-openvpn/pull/161)) HISTORY.md:* Support for systemd ([#127](https://github.com/luxflux/puppet-openvpn/pull/127)) data/defaults.yaml:openvpn::systemd: false manifests/init.pp: if $facts['service_provider'] != 'systemd' { manifests/server.pp: if $facts['service_provider'] == 'systemd' and $openvpn::namespecific_rclink { manifests/server.pp: fail("Using systemd and namespecific rclink's (BSD-style) is not allowed") manifests/server.pp: if $facts['service_provider'] == 'systemd' { manifests/server.pp: if $facts['service_provider'] == 'systemd' { manifests/server.pp: provider => 'systemd', spec/classes/openvpn_init_spec.rb: context 'system without systemd' do spec/classes/openvpn_init_spec.rb: service_provider: 'systemd' spec/classes/openvpn_init_spec.rb: context 'system with systemd' do

bastelfreak · 2018-10-21T06:54:21Z

Fixes #306

bastelfreak · 2018-10-21T06:57:17Z

Thanks for the awesome PR @jkroepke. I only did some little inline comments, but I didn't spot anything bad. Can you also add Ubuntu 18.04 to our acceptance test matrix? That would be:

diff --git a/.sync.yml b/.sync.yml
index d485814..e241dff 100644
--- a/.sync.yml
+++ b/.sync.yml
@@ -1,6 +1,7 @@
 ---
 .travis.yml:
   docker_sets:
+    - set: ubuntu1804-64
     - set: ubuntu1604-64
     - set: ubuntu1404-64
     - set: centos7-64
diff --git a/.travis.yml b/.travis.yml
index 1b0d04c..3566dc8 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -25,6 +25,24 @@ matrix:
   - rvm: 2.4.4
     bundler_args: --without system_tests development release
     env: PUPPET_VERSION="~> 5.0" CHECK=build DEPLOY_TO_FORGE=yes
+  - rvm: 2.5.1
+    bundler_args: --without development release
+    dist: trusty
+    env: PUPPET_INSTALL_TYPE=agent BEAKER_IS_PE=no BEAKER_PUPPET_COLLECTION=puppet5 BEAKER_debug=true BEAKER_setfile=ubuntu1804-64{hypervisor=docker} CHECK=beaker
+    services: docker
+    sudo: required
+  - rvm: 2.5.1
+    bundler_args: --without development release
+    dist: trusty
+    env: PUPPET_INSTALL_TYPE=agent BEAKER_IS_PE=no BEAKER_PUPPET_COLLECTION=puppet6 BEAKER_debug=true BEAKER_setfile=ubuntu1804-64{hypervisor=docker} CHECK=beaker
+    services: docker
+    sudo: required
+  - rvm: 2.5.1
+    bundler_args: --without development release
+    dist: trusty
+    env: PUPPET_INSTALL_TYPE=agent BEAKER_IS_PE=no BEAKER_PUPPET_COLLECTION=puppet6-nightly BEAKER_debug=true BEAKER_setfile=ubuntu1804-64{hypervisor=docker} CHECK=beaker
+    services: docker
+    sudo: required
   - rvm: 2.5.1
     bundler_args: --without development release
     dist: trusty

If that doesn't work we can still implement it in another PR, to keep the diff small in this one.

jkroepke · 2018-10-21T08:58:28Z

Thanks for the reviews!

@bastelfreak about the acceptance test. I would add it but I can not find a nodeset for 1804 here. Is this correct?

https://github.com/voxpupuli/puppet-openvpn/tree/master/spec/acceptance/nodesets

bastelfreak · 2018-10-21T09:04:03Z

The nodesets for docker are generated on the fly. It's triggered by BEAKER_setfile=ubuntu1804-64{hypervisor=docker} which utilities https://github.com/puppetlabs/beaker-hostgenerator#beaker-host-generator.

* Add Data in modules * Remove root_group parameter * Removed top scope variables * Removed create_resources * Removed params.pp * Remove systemd option in favor of facts provided by stdlib

manifests/config.pp

bastelfreak · 2018-10-21T09:29:13Z

manifests/client.pp

@@ -354,7 +354,7 @@

  file { "${etc_directory}/openvpn/${server}/download-configs/${name}/${name}.conf":
    owner   => root,
-    group   => $::openvpn::params::root_group,
+    group   => 0,


Why did you switch away from the parameter to a hardcoded number? I don't think that there are a lot of usecases to set this to something else, but since the parameter is already there we could keep it?

I would prefer to keep it, because the root group is always 0.

But its backwards-incompatible, expect you want this parameter.

manifests/ca.pp

Co-Authored-By: jkroepke <[email protected]>

manifests/ca.pp

Co-Authored-By: jkroepke <[email protected]>

jkroepke · 2018-10-21T09:37:52Z

manifests/init.pp

    class { 'openvpn::service':
      subscribe => [Class['openvpn::config'], Class['openvpn::install'] ],
    }
+
    if empty($servers) {


@bastelfreak What about this?

It can be just if !$servers or if $servers != {}

mhm, good question. We don't have anything for this in our styleguide. On a first guess empty($servers) looks as good as if !$servers.

bastelfreak · 2018-10-21T13:13:41Z

I am fine with this as is, but would like to see another review

Dan33l · 2018-10-22T13:06:57Z

Interesting PR, thank you.
I didn't saw weird things and i am fine also.

jkroepke force-pushed the data_in_modules branch 2 times, most recently from 3112cad to ed49b84 Compare October 20, 2018 22:56

jkroepke changed the title ~~WIP: Data in Modules~~ WIP: Data in Modules & Cleanup Oct 20, 2018

jkroepke force-pushed the data_in_modules branch 2 times, most recently from 532fb76 to 0ea62ba Compare October 20, 2018 23:12

bastelfreak reviewed Oct 21, 2018

View reviewed changes

bastelfreak added enhancement New feature or request needs-work not ready to merge just yet labels Oct 21, 2018

jkroepke force-pushed the data_in_modules branch 2 times, most recently from 033756a to b28d69b Compare October 21, 2018 09:05

* Add defaults for unsupported OS

2e981b9

* Add Data in modules * Remove root_group parameter * Removed top scope variables * Removed create_resources * Removed params.pp * Remove systemd option in favor of facts provided by stdlib

jkroepke force-pushed the data_in_modules branch from b28d69b to 2e981b9 Compare October 21, 2018 09:13

jkroepke changed the title ~~WIP: Data in Modules & Cleanup~~ Data in Modules, Modern facts & Cleanup Oct 21, 2018

jkroepke commented Oct 21, 2018

View reviewed changes

manifests/config.pp Show resolved Hide resolved

bastelfreak reviewed Oct 21, 2018

View reviewed changes

manifests/ca.pp Outdated Show resolved Hide resolved

Simplify if condition

0dc3b5d

Co-Authored-By: jkroepke <[email protected]>

bastelfreak reviewed Oct 21, 2018

View reviewed changes

manifests/ca.pp Outdated Show resolved Hide resolved

Simplify if condition

97f2cb3

Co-Authored-By: jkroepke <[email protected]>

jkroepke commented Oct 21, 2018

View reviewed changes

bastelfreak approved these changes Oct 21, 2018

View reviewed changes

bastelfreak added backwards-incompatible and removed needs-work not ready to merge just yet labels Oct 21, 2018

bastelfreak merged commit e105b2f into voxpupuli:master Oct 24, 2018

jkroepke deleted the data_in_modules branch October 24, 2018 20:40

Dan33l mentioned this pull request Dec 2, 2018

Implement Ubuntu 18.04 support #306

Closed

Dan33l mentioned this pull request Jan 4, 2019

New release ? #323

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Data in Modules, Modern facts & Cleanup #305

Data in Modules, Modern facts & Cleanup #305

jkroepke commented Oct 20, 2018 •

edited

Loading

bastelfreak Oct 21, 2018

bastelfreak Oct 21, 2018

bastelfreak Oct 21, 2018

bastelfreak Oct 21, 2018

bastelfreak Oct 21, 2018

bastelfreak Oct 21, 2018

bastelfreak Oct 21, 2018

bastelfreak commented Oct 21, 2018

bastelfreak commented Oct 21, 2018

jkroepke commented Oct 21, 2018

bastelfreak commented Oct 21, 2018

bastelfreak Oct 21, 2018

jkroepke Oct 21, 2018

jkroepke Oct 21, 2018

bastelfreak Oct 21, 2018

bastelfreak commented Oct 21, 2018

Dan33l commented Oct 22, 2018

	Variant[String, Array] $additional_packages,
	Variant[String[1], Array[String[1]]] $additional_packages,

	if $openvpn::additional_packages != undef {
	if $openvpn::additional_packages {

Data in Modules, Modern facts & Cleanup #305

Data in Modules, Modern facts & Cleanup #305

Conversation

jkroepke commented Oct 20, 2018 • edited Loading

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

Tested

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

bastelfreak commented Oct 21, 2018

bastelfreak commented Oct 21, 2018

jkroepke commented Oct 21, 2018

bastelfreak commented Oct 21, 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

bastelfreak commented Oct 21, 2018

Dan33l commented Oct 22, 2018

jkroepke commented Oct 20, 2018 •

edited

Loading