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

Refactor to simplify code #53

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Refactor to simplify code #53

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 5 additions & 6 deletions examples/init.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,14 @@

include caddy


file {'/var/www':
file { '/var/www':
ensure => directory,
}

caddy::vhost {'example1':
source => 'puppet:///modules/caddy/etc/caddy/config/example1.conf',
caddy::vhost { 'example1':
content => file('caddy/examples/example1.conf'),
Copy link
Member

Choose a reason for hiding this comment

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

Given its size, wouldn't it be clearer to use heredoc and inline it in the example?

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 pretty much useless, because we already have it at manifests/vhosts.pp and REFERENCE.md for what it is worth.

Copy link
Member

Choose a reason for hiding this comment

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

One pattern I've been using is to read examples in acceptance tests: https://github.com/theforeman/puppet-foreman_proxy/blob/d2f07f4a7e460231015f91469f298eb6831c7479/spec/spec_helper_acceptance.rb#L33-L40

It's on my agenda to add that example to voxpupuli-acceptance. The benefit is that you have syntax highlighting and linting for your code samples and you can browse the things we test.

So for now I wouldn't just get rid of it.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ekohl we do the same thing for sensu-puppet

https://github.com/sensu/sensu-puppet/blob/master/spec/spec_helper_acceptance.rb#L46
https://github.com/sensu/sensu-puppet/blob/master/spec/acceptance/examples_spec.rb

Copy link
Member Author

@ghoneycutt ghoneycutt Apr 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I would prefer the HEREDOC approach if only to get rid of the example data from the module under files/. Really that should only include what is actually used by the module. Given it is named examples though, it does seem very clear what is happening. I'll let @dhoppe flip a coin and decide to change it or not in a later PR :)

Copy link
Member

Choose a reason for hiding this comment

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

@ekohl we do the same thing for sensu-puppet

Glad to hear I'm not alone in this. Given the code is very different, this clearly evolved separate which tells me there's a good chance it's a good idea :)

I would prefer the HEREDOC approach if only to get rid of the example data from the module under files/

That was indeed my reason for it.

}

caddy::vhost {'example2':
source => 'puppet:///modules/caddy/etc/caddy/config/example2.conf',
caddy::vhost { 'example2':
content => file('caddy/examples/example2.conf'),
}
0 files/etc/caddy/Caddyfile → files/Caddyfile
View file
Open in desktop
File renamed without changes.
0 files/etc/caddy/config/example1.conf → files/examples/example1.conf
View file
Open in desktop
File renamed without changes.
0 files/etc/caddy/config/example2.conf → files/examples/example2.conf
View file
Open in desktop
File renamed without changes.
46 changes: 0 additions & 46 deletions manifests/config.pp
View file
Open in desktop

This file was deleted.

155 changes: 149 additions & 6 deletions manifests/init.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,9 @@
Optional[String[1]] $systemd_ambient_capabilities = undef,
Optional[Boolean] $systemd_no_new_privileges = undef,
) {

include file_capability

case $caddy_architecture {
'x86_64', 'amd64': { $arch = 'amd64'}
'x86' : { $arch = '386' }
Expand All @@ -118,6 +121,19 @@
}
}

case $install_method {
'github': {
$caddy_url = 'https://github.com/caddyserver/caddy/releases/download'
$caddy_dl_url = "${caddy_url}/v${version}/caddy_v${version}_linux_${arch}.tar.gz"
$caddy_dl_dir = "${caddy_tmp_dir}/caddy_v${version}_linux_${$arch}.tar.gz"
}
default: {
$caddy_url = 'https://caddyserver.com/download/linux'
$caddy_dl_url = "${caddy_url}/${arch}?plugins=${caddy_features}&license=${caddy_license}&telemetry=${caddy_telemetry}"
$caddy_dl_dir = "${caddy_tmp_dir}/caddy_linux_${$arch}_custom.tar.gz"
}
}

group { $caddy_group:
ghoneycutt marked this conversation as resolved.
Show resolved Hide resolved
ensure => present,
system => true,
Expand All @@ -131,11 +147,138 @@
home => $caddy_home,
}

contain caddy::install
contain caddy::config
contain caddy::service
file { $install_path:
ensure => directory,
owner => $caddy_user,
group => $caddy_group,
mode => '0755',
}

archive { $caddy_dl_dir:
ensure => present,
extract => true,
extract_path => $install_path,
source => $caddy_dl_url,
username => $caddy_account_id,
password => $caddy_api_key,
user => 'root',
group => 'root',
creates => "${install_path}/caddy",
cleanup => true,
notify => File_capability["${install_path}/caddy"],
require => File[$install_path],
}

file_capability { "${install_path}/caddy":
ensure => present,
capability => 'cap_net_bind_service=ep',
require => Archive[$caddy_dl_dir],
}

file { $caddy_home:
ensure => directory,
owner => $caddy_user,
group => $caddy_group,
mode => '0755',
require => Archive[$caddy_dl_dir],
notify => Service['caddy'],
}

file { $caddy_ssl_dir:
ensure => directory,
owner => $caddy_user,
group => $caddy_group,
mode => '0755',
require => Archive[$caddy_dl_dir],
notify => Service['caddy'],
}

file { $caddy_log_dir:
ensure => directory,
owner => $caddy_user,
group => $caddy_group,
mode => '0755',
require => Archive[$caddy_dl_dir],
notify => Service['caddy'],
}

file { '/etc/caddy':
ensure => directory,
owner => 'root',
group => 'root',
mode => '0755',
require => Archive[$caddy_dl_dir],
notify => Service['caddy'],
}

file { '/etc/caddy/Caddyfile':
ensure => file,
owner => $caddy_user,
group => $caddy_group,
mode => '0444',
content => file('caddy/Caddyfile'),
require => Archive[$caddy_dl_dir],
notify => Service['caddy'],
}

file { '/etc/caddy/config':
ensure => directory,
purge => true,
recurse => true,
owner => $caddy_user,
group => $caddy_group,
mode => '0755',
require => Archive[$caddy_dl_dir],
notify => Service['caddy'],
}

Class['caddy::install']
-> Class['caddy::config']
~> Class['caddy::service']
case $facts['service_provider'] {
default: {
fail("service provider ${$facts['service_provider']} is not supported.")
}
'systemd': {
systemd::unit_file { 'caddy.service':
content => epp('caddy/caddy.service.epp',
{
install_path => $install_path,
caddy_user => $caddy_user,
caddy_group => $caddy_group,
caddy_log_dir => $caddy_log_dir,
caddy_ssl_dir => $caddy_ssl_dir,
caddy_home => $caddy_home,
caddy_http_port => $caddy_http_port,
caddy_https_port => $caddy_https_port,
systemd_limit_processes => $systemd_limit_processes,
systemd_private_devices => $systemd_private_devices,
systemd_capability_bounding_set => $systemd_capability_bounding_set,
systemd_ambient_capabilities => $systemd_ambient_capabilities,
systemd_no_new_privileges => $systemd_no_new_privileges,
}
),
notify => Service['caddy'],
}
}
'redhat': {
file { '/etc/init.d/caddy':
ensure => file,
content => epp('caddy/caddy.epp',
{
caddy_user => $caddy_user,
caddy_log_dir => $caddy_log_dir,
caddy_ssl_dir => $caddy_ssl_dir,
caddy_home => $caddy_home,
}
),
owner => 'root',
group => 'root',
mode => '0755',
notify => Service['caddy'],
}
}
}

service { 'caddy':
ensure => running,
enable => true,
}
}
64 changes: 0 additions & 64 deletions manifests/install.pp
View file
Open in desktop

This file was deleted.

72 changes: 0 additions & 72 deletions manifests/service.pp
View file
Open in desktop

This file was deleted.

7 changes: 5 additions & 2 deletions manifests/vhost.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,15 @@
) {

include caddy
ghoneycutt marked this conversation as resolved.
Show resolved Hide resolved

file { "/etc/caddy/config/${title}.conf":
ensure => file,
content => $content,
source => $source,
owner => $caddy::caddy_user,
group => $caddy::caddy_group,
mode => '0444',
require => Class['caddy::config'],
ghoneycutt marked this conversation as resolved.
Show resolved Hide resolved
notify => Class['caddy::service'],
require => File['/etc/caddy/Caddyfile'],
notify => Service['caddy'],
}
}
Loading