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

initial support for snippets #1231

Merged
merged 1 commit into from
Feb 9, 2019
Merged

Conversation

bryangwilliam
Copy link
Contributor

@bryangwilliam bryangwilliam commented Jul 10, 2018

Pull Request (PR) description

Add basic support for creating snippets (reusable configuration files) that can be included by other resources. In phase 1 of this effort I am just introducing the ability to create include files by providing the raw contents, but future phases will include writing server::resource::location output into a snippet for reusability.

Please let me know your thoughts on the direction this is going.

This Pull Request (PR) fixes the following issues

Fixes #644
Fixes #1135

@LongLiveCHIEF
Copy link

LongLiveCHIEF commented Jul 27, 2018

If we're going to add this in, we need to have a way where we can also ensure the snippets directory is absent when manage snippets is set to true.

We will also need tests and docs added for this functionality.

@LongLiveCHIEF LongLiveCHIEF added enhancement New feature or request needs-tests needs-docs needs-work not ready to merge just yet labels Jul 27, 2018
@LongLiveCHIEF
Copy link

Would you also need to notify the service for any new snippets added? This isn't a feature I've used from nginx before, so just asking. If yes, then you'll want to work that in before we pull this.

@LongLiveCHIEF LongLiveCHIEF self-requested a review July 27, 2018 21:05
Copy link

@LongLiveCHIEF LongLiveCHIEF left a comment

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 all the ::nginx and replace them with nginx? We have another PR in to finally update all this throughout the rest of the module and I don't want to re-introduce them here.

templates/snippet/snippet_header.erb Outdated Show resolved Hide resolved
@LongLiveCHIEF
Copy link

LongLiveCHIEF commented Jul 27, 2018

Any new templates added have to be .epp. Can you make this change?

Nevermind, you got it covered, i just missed it in my first read-through.

@LongLiveCHIEF
Copy link

Ok, so a recap of changes needed before we can pull:

  • remove ::nginx and substitute with nginx, per current standards
  • switch the new template introduced from an erb to an epp (again current standards)
  • add a way for users to set absent when snippets dir is managed
  • add tests
  • add docs

@bastelfreak bastelfreak force-pushed the snippets branch 5 times, most recently from 86726fd to aa07f14 Compare February 9, 2019 19:05
manifests/resource/snippet.pp Outdated Show resolved Hide resolved
$name_sanitized = regsubst($name, ' ', '_', 'G')
$config_file = "${nginx::snippets_dir}/${name_sanitized}.conf"

concat { $config_file:
Copy link
Member

Choose a reason for hiding this comment

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

Probably need a check in here to ensure that $config_file isn't already defined as a concat resource. If it is and you try to add another snippet to the same $config_file, it will fail when it tries to create this resource.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about this. $config_file contains the name of the defined resource, so it's already unique in the catalog.

@bastelfreak bastelfreak removed needs-docs needs-tests needs-work not ready to merge just yet labels Feb 9, 2019
@bastelfreak bastelfreak merged commit 077fc6c into voxpupuli:master Feb 9, 2019
cegeka-jenkins pushed a commit to cegeka/puppet-nginx that referenced this pull request Sep 13, 2019
Rubueno pushed a commit to Rubueno/puppet-nginx that referenced this pull request Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: assign nginx location to multiple servers Same location on multiple vhosts
4 participants