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

Add support for puppetlabs/strings #48

Closed
wants to merge 8 commits into from
Closed

Add support for puppetlabs/strings #48

wants to merge 8 commits into from

Conversation

dhoppe
Copy link
Member

@dhoppe dhoppe commented Apr 28, 2020

Fixes #33, #44

@dhoppe dhoppe requested a review from bastelfreak April 28, 2020 10:44
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Overall 👍

@@ -3,6 +3,7 @@
docker_sets:
- set: centos6-64
- set: centos7-64
- set: centos8-64
Copy link
Member

Choose a reason for hiding this comment

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

This looks unrelated to strings

Copy link
Member Author

Choose a reason for hiding this comment

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

That was missing at a pull request I merged earlier today.


### Setup Requirements

This module has the following dependencies:
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to list these? IMHO metadata.json is a better place.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have no idea. We have these dependencies at metadata.json, but it is nice to get an quick overview without jumping through all these files.

However, it is almost impossible to create a good README.md, because all of our modules use a different style and we do not have a reference module like puppetlabs/puppetlabs-apt.

String $caddy_features = 'http.filter,http.git,http.ipfilter',
Stdlib::Port $caddy_http_port = 80,
Stdlib::Port $caddy_https_port = 443,
Stdlib::Absolutepath $install_path = '/usr/local/bin',
Copy link
Member

Choose a reason for hiding this comment

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

I have stopped aligning because I think it just makes things hard to maintain. Won't object, but I wouldn't bother.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this mess came with the data types, but I think it is easier to read this way.

'redhat': { # we could probably add 'debian' for older debian releases but not sure
file {'/etc/init.d/caddy':
'redhat': {
file { '/etc/init.d/caddy':
ensure => file,
mode => '0744',
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated, but what a weird mode. 0755 is much more logical IMHO.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this was already in here and nobody cared about it. I will change it. ;)

#
define caddy::vhost(
$source = undef,
$content = undef,
$source = undef,
Copy link
Member

Choose a reason for hiding this comment

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

Undocumented, also

Suggested change
$source = undef,
Optional[Stdlib::Filesource] $source = undef,

Copy link
Member Author

Choose a reason for hiding this comment

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

Thx. I missed that somehow.

$source = undef,
$content = undef,
$source = undef,
$content = undef,
Copy link
Member

Choose a reason for hiding this comment

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

Undocumented, also

Suggested change
$content = undef,
Optional[String] $content = undef,

Copy link
Member Author

Choose a reason for hiding this comment

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

Thx. I missed that somehow.

@dhoppe dhoppe requested a review from ekohl April 28, 2020 13:35
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I think you need to regenerate REFERENCE.md

@dhoppe
Copy link
Member Author

dhoppe commented Apr 28, 2020

@ekohl Just generated a new REFERENCE.md.

@vox-pupuli-tasks
Copy link

Dear @dhoppe, thanks for the PR!

This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

1 similar comment
@vox-pupuli-tasks
Copy link

Dear @dhoppe, thanks for the PR!

This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

@ghoneycutt
Copy link
Member

Seems to duplicate #49 which was merged. Please re-open if that's not the case.

@ghoneycutt ghoneycutt closed this Apr 28, 2020
@dhoppe dhoppe deleted the add_reference branch April 28, 2020 22:29
@dhoppe dhoppe changed the title Add support for puppetlabs/puppetlabs-strings Add support for puppetlabs/strings Apr 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for puppetlabs/puppet-strings
3 participants