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 'require' for parent dir of upstream, map, and geo configs as wel… #958

Merged
merged 1 commit into from
Nov 4, 2016
Merged

Add 'require' for parent dir of upstream, map, and geo configs as wel… #958

merged 1 commit into from
Nov 4, 2016

Conversation

wyardley
Copy link
Collaborator

@wyardley wyardley commented Nov 2, 2016

I think this should come a lot closer to resolving #942.

Few questions:

  1. Should these be class specs, not defines? And should I have one test for the file existing, and another for it existing and having the require, or is this fine?
  2. Should I avoid using $conf_dir within the resource manifests (since we've got $::nginx::config::conf_dir? Or is this fine?
  3. I went back and forth on making some of these variables within the scope of $::nginx::config, but decided to leave it this way, but open to discussion. Upstream and vhost specifically differs depending on settings like $confd_only, and some of the other resources have their own directories, like conf.mail.d, so I'm not sure if making it more generic is worth the effort.

@dhoppe
Copy link
Member

dhoppe commented Nov 4, 2016

I think this PR is just fine and has already been tested via #942.

@wyardley wyardley merged commit 0210c96 into voxpupuli:master Nov 4, 2016
cegeka-jenkins pushed a commit to cegeka/puppet-nginx that referenced this pull request Sep 13, 2019
…le_requires

Add 'require' for parent dir of upstream, map, and geo configs as wel…
Rubueno pushed a commit to Rubueno/puppet-nginx that referenced this pull request Oct 19, 2020
…le_requires

Add 'require' for parent dir of upstream, map, and geo configs as wel…
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants