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

Jump to bottom

Add support for dynamic modules. #1180

Merged
merged 1 commit into from
May 22, 2018
Merged

Add support for dynamic modules. #1180

merged 1 commit into from
May 22, 2018

Conversation

sevencastles
Copy link
Contributor

Allow nginx to load dynamic modules by specifying an absolute or a
relative ("modules/") path. Modules must be installed separately and
before loading the puppet nginx module.

@eputnam eputnam added the enhancement New feature or request label Feb 26, 2018
bastelfreak
bastelfreak reviewed Mar 3, 2018
View reviewed changes
manifests/init.pp Outdated
@@ -33,6 +33,7 @@
Optional[Enum['on', 'off']] $daemon = undef,
$daemon_user = $::nginx::params::daemon_user,
$daemon_group = undef,
Optional[Array[String]] $dynamic_module = undef,
Copy link
Member

Choose a reason for hiding this comment

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

Can you update it so it defaults to an empty array?

Copy link
Member

Choose a reason for hiding this comment

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

I'd also prefer it to be named dynamic_modules since it's an array.

bastelfreak
bastelfreak reviewed Mar 3, 2018
View reviewed changes
spec/classes/nginx_spec.rb
@@ -337,6 +337,12 @@

describe 'nginx.conf template content' do
[
{
title: 'should not set load_module',
Copy link
Member

Choose a reason for hiding this comment

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

can you add another test that sets a module and checks if it is present in the config file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is done, check lines 891-910

bastelfreak
bastelfreak reviewed Mar 3, 2018
View reviewed changes
templates/conf.d/nginx.conf.erb Outdated
@@ -1,4 +1,13 @@
# MANAGED BY PUPPET
<% if @dynamic_module.is_a?(Array) -%>
Copy link
Member

Choose a reason for hiding this comment

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

the variable is always an array, so you don't need this check

@bastelfreak
Copy link
Member

Hi @sevencastles, thanks for the PR! I added a few inline comments, please take a look at them. The used email address in your commit doesn't match the one you've configured in your github settings. Now the commit isn't associated with your account. Can you please fix that?

@bastelfreak bastelfreak added the needs-work not ready to merge just yet label Mar 3, 2018
@sevencastles sevencastles deleted the dynamic_modules branch March 6, 2018 13:12
@sevencastles sevencastles restored the dynamic_modules branch March 6, 2018 13:14
@sevencastles sevencastles reopened this Mar 6, 2018
@sevencastles
Copy link
Contributor Author

Hi,
thanks a lot for your suggestions. I have applied all of them.

@bastelfreak
Copy link
Member

Thanks for the updates @sevencastles . Can you take a look at #1180 (comment) ?
Also the email address in the first commit isn't linked with your github account. You can squash those commits into one and reset the author. Some explanations: https://stackoverflow.com/questions/3042437/change-commit-author-at-one-specific-commit

@sevencastles sevencastles force-pushed the dynamic_modules branch 2 times, most recently from 0bc7a54 to fe6606a Compare March 6, 2018 14:53
@sevencastles
Copy link
Contributor Author

Hi @bastelfreak. I've squashed the commits into one. Also, I've (already) added the tests you've asked, if I have understood you right.

bastelfreak
bastelfreak reviewed Mar 6, 2018
View reviewed changes
templates/conf.d/nginx.conf.erb Outdated
@@ -1,4 +1,11 @@
# MANAGED BY PUPPET
<% Array(@dynamic_modules).each do |mod_item| -%>
Copy link
Member

Choose a reason for hiding this comment

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

Can you get rid of the Array()?
That isn't needed since it's always an array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will do. Sorry, I am not very good with ERB

@sevencastles
Copy link
Contributor Author

Hi,
do you have any news? I have implemented all the requested changes.

@sevencastles
Copy link
Contributor Author

I've updated my pull request to be compatible with the latest master. Can you have a look and merge it? @bastelfreak

Thanks

alexjfisher
alexjfisher reviewed May 22, 2018
View reviewed changes
manifests/init.pp Outdated
@@ -33,6 +33,7 @@
Optional[Enum['on', 'off']] $daemon = undef,
$daemon_user = $nginx::params::daemon_user,
$daemon_group = undef,
Optional[Array[String]] $dynamic_modules = [],
Copy link
Member

Choose a reason for hiding this comment

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

If not defaulting to undef, do you really need to use Optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess I can indeed remove Optional. Will do

@sevencastles
Add support for dynamic modules.
e7199da 
Allow nginx to load dynamic modules by specifying an absolute or a
relative ("modules/") path. Modules must be installed separately and
before loading the puppet nginx module.
@alexjfisher alexjfisher removed the needs-work not ready to merge just yet label May 22, 2018
@bastelfreak bastelfreak merged commit 8765ef8 into voxpupuli:master May 22, 2018
Rubueno pushed a commit to Rubueno/puppet-nginx that referenced this pull request Oct 19, 2020
@bastelfreak
Merge pull request voxpupuli#1180 from sevencastles/dynamic_modules
137c493 
Add support for dynamic modules.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bastelfreak bastelfreak bastelfreak left review comments

@ekohl ekohl ekohl left review comments

@alexjfisher alexjfisher alexjfisher approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sevencastles @bastelfreak @alexjfisher @ekohl @eputnam