add openvpn::deploy::(export/client) #261

to-kn · 2018-01-06T16:48:51Z

proposal to integrate https://bitbucket.org/codacity/puppet-module-openvpn_client as mentioned in #231

bastelfreak · 2018-01-06T16:52:10Z

lib/facter/openvpn_deploy_cert_data.rb

+Facter.add("openvpn::deploy_cert_data") do
+  setcode do
+    clients = {}
+    path = '/etc/openvpn'


are there any operatingsystems we support, that use a different path?

bastelfreak · 2018-01-06T16:52:32Z

manifests/deploy/client.pp

+#
+
+define openvpn::deploy::client (
+  String $server,


Datatypes \o/

bastelfreak · 2018-01-06T16:53:17Z

manifests/deploy/client.pp

+  ~> Class['openvpn::deploy::service']
+
+
+  if ($manage_etc == true) {


We can be lazy here and save a few chars:

if $manage_etc {}

bastelfreak · 2018-01-06T16:53:31Z

manifests/deploy/client.pp

+  File <<| tag == "${server}-${name}" |>>
+  ~> Class['openvpn::deploy::service']
+
+}


please add a newline here

bastelfreak · 2018-01-06T16:53:52Z

manifests/deploy/export.pp

+      tag     => "${server}-${name}",
+    }
+  }
+}


please add a newline here

bastelfreak · 2018-01-06T16:54:15Z

manifests/deploy/install.pp

+
+  ensure_packages(['openvpn'])
+
+}


bastelfreak · 2018-01-06T16:54:37Z

manifests/deploy/install.pp

+
+class openvpn::deploy::install {
+
+  ensure_packages(['openvpn'])


isn't that already covered by this module?

on the server yes, on the client not as far as i am aware of...

bastelfreak · 2018-01-06T16:55:32Z

manifests/deploy/service.pp

+
+class openvpn::deploy::service {
+
+  service { 'openvpn':


this is also redundant with https://github.com/voxpupuli/puppet-openvpn/blob/master/manifests/service.pp ?

i am not shure, if i should/could reuse the server part for the client here too? (same as for install)

bastelfreak · 2018-01-06T16:56:14Z

Hi @to-kn, thanks for the PR. I added a few inline comments. Can you take a look a them and add a few spec tests?

to-kn · 2018-01-07T16:04:37Z

hi @bastelfreak, like that?

fix linting, add credit, add tests fixes voxpupuli#231

bastelfreak · 2018-01-07T23:12:06Z

Can you checked the failed travis run?

to-kn · 2018-01-10T21:39:48Z

@bastelfreak looks good now!

bastelfreak · 2018-01-10T21:54:41Z

lib/facter/openvpn.rb

+  # @return [NilClass]
+  def self.add_facts
+    certs = client_certs
+    Facter.add('openvpn::client_configs') do


Could we use a different delimiter here and not ::? This looks to much like a normal puppet variable.

bastelfreak · 2018-01-10T21:55:47Z

manifests/deploy/export.pp

+  -> Openvpn::Client[$name]
+  -> Openvpn::Deploy::Export[$name]
+
+  if $facts['openvpn::client_configs'] {


on a first view, this looks like you lookup a puppet variable in the facts hash, I think we should change it.

would you prefer something along the lines of "openvpn_client_configs"?

openvpn_client_configs or just openvpn or openvpn {client_configs{}} sound good I guess

bastelfreak · 2018-01-10T21:57:00Z

spec/spec_helper.rb

@@ -2,6 +2,8 @@
 require 'rspec-puppet-facts'
 include RspecPuppetFacts

+$LOAD_PATH.unshift(File.expand_path(File.dirname(__FILE__) + '/../'))


this will get killed by the next modulesync. This file is a template. Is this actually needed?

if i remove that line, than i get an error while running the tests. - maybe you know, how this could be fixed?

An error occurred while loading ./spec/unit/openvpn_module_spec.rb. Failure/Error: require 'lib/facter/openvpn' LoadError: cannot load such file -- lib/facter/openvpn

What about moving that $LOAD_PATH thing into spec/unit/openvpn_module_spec.rb? That may work.

bastelfreak · 2018-01-10T21:58:19Z

spec/unit/openvpn_module_spec.rb

@@ -0,0 +1,103 @@
+require 'spec_helper'


Uh sweet. Good job in that file!

bastelfreak · 2018-01-10T22:02:53Z

spec/spec_helper.rb

@@ -27,6 +29,7 @@
  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.hiera_config = 'spec/fixtures/hiera/hiera.yaml'
+  c.mock_with :rspec


I thought rspec is the default and this isn't needed. However, our template actually supports it \o/
https://github.com/voxpupuli/modulesync_config/blob/master/moduleroot/spec/spec_helper.rb.erb#L47

Please configure it like this: https://github.com/voxpupuli/puppet-extlib/blob/master/.sync.yml#L5

add mock_with to .sync.yml and remove additional $LOAD from spec_helper

to-kn · 2018-01-10T22:30:16Z

i also added a short info to the readme

bastelfreak · 2018-01-11T23:00:08Z

Thanks for all the work @to-kn!

bastelfreak reviewed Jan 6, 2018

View reviewed changes

manifests/deploy/client.pp

#

define openvpn::deploy::client (

String $server,

Copy link

Member

bastelfreak Jan 6, 2018

Choose a reason for hiding this comment

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

Datatypes \o/

bastelfreak reviewed Jan 6, 2018

View reviewed changes

manifests/deploy/export.pp Outdated

tag => "${server}-${name}",

}

}

}

Copy link

Member

bastelfreak Jan 6, 2018

Choose a reason for hiding this comment

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

please add a newline here

bastelfreak reviewed Jan 6, 2018

View reviewed changes

manifests/deploy/install.pp

ensure_packages(['openvpn'])

}

Copy link

Member

bastelfreak Jan 6, 2018

Choose a reason for hiding this comment

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

newline

bastelfreak reviewed Jan 6, 2018

View reviewed changes

bastelfreak added enhancement New feature or request needs-tests labels Jan 6, 2018

to-kn force-pushed the feat_deploy branch 2 times, most recently from 439cbe3 to 89cd5b4 Compare January 7, 2018 15:58

add openvpn::deploy::(export/client)

d0fd9f3

fix linting, add credit, add tests fixes voxpupuli#231

to-kn force-pushed the feat_deploy branch from 89cd5b4 to d0fd9f3 Compare January 7, 2018 18:47

to-kn added 2 commits January 8, 2018 00:38

test if that fixes travis in ruby 2.1.9

ff5cd1d

test if that fixes travis in ruby 2.1.9

e1e063c

bastelfreak reviewed Jan 10, 2018

View reviewed changes

change name of fact to "openvpn"

9f3eb23

add mock_with to .sync.yml and remove additional $LOAD from spec_helper

bastelfreak removed the needs-tests label Jan 10, 2018

add short example to documentation

085eb26

bastelfreak merged commit 652f33a into voxpupuli:master Jan 11, 2018

jkroepke mentioned this pull request Mar 8, 2022

Remove openvpn::deploy #424

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add openvpn::deploy::(export/client) #261

add openvpn::deploy::(export/client) #261

to-kn commented Jan 6, 2018

bastelfreak Jan 6, 2018

bastelfreak Jan 6, 2018

bastelfreak Jan 6, 2018

bastelfreak Jan 6, 2018

bastelfreak Jan 6, 2018

bastelfreak Jan 6, 2018

bastelfreak Jan 6, 2018

to-kn Jan 6, 2018

bastelfreak Jan 6, 2018

to-kn Jan 6, 2018

bastelfreak commented Jan 6, 2018

to-kn commented Jan 7, 2018

bastelfreak commented Jan 7, 2018

to-kn commented Jan 10, 2018

bastelfreak Jan 10, 2018

bastelfreak Jan 10, 2018

to-kn Jan 10, 2018

bastelfreak Jan 10, 2018

bastelfreak Jan 10, 2018

to-kn Jan 10, 2018 •

edited

Loading

bastelfreak Jan 10, 2018

bastelfreak Jan 10, 2018

to-kn Jan 10, 2018

bastelfreak Jan 10, 2018

to-kn commented Jan 10, 2018

bastelfreak commented Jan 11, 2018

		~> Class['openvpn::deploy::service']


		if ($manage_etc == true) {


		class openvpn::deploy::install {

		ensure_packages(['openvpn'])

add openvpn::deploy::(export/client) #261

add openvpn::deploy::(export/client) #261

Conversation

to-kn commented Jan 6, 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

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 Jan 6, 2018

to-kn commented Jan 7, 2018

bastelfreak commented Jan 7, 2018

to-kn commented Jan 10, 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

Choose a reason for hiding this comment

to-kn Jan 10, 2018 • edited Loading

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

to-kn commented Jan 10, 2018

bastelfreak commented Jan 11, 2018

to-kn Jan 10, 2018 •

edited

Loading