-
-
Notifications
You must be signed in to change notification settings - Fork 880
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
Require base folder for resources #624
Conversation
@@ -70,6 +70,7 @@ | |||
concat { "${::nginx::config::conf_dir}/conf.d/${name}-upstream.conf": | |||
ensure => $ensure_real, | |||
notify => Class['::nginx::service'], | |||
require => File["${::nginx::config::conf_dir}/conf.d/"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're requiring a file here that doesn't exist. You should require File["${::nginx::config::conf_dir}/conf.d"]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also please ensure the =>
arrows are aligned.
Hi @Tombar did you test that this change in fact solved your issue? The proposed change to upstream.pp would have resulted in an error when you tried in your environment. Also can I ask which version of concat you were using that failed, and the last version that worked? And your Puppet version? From what I've read this may be a permissions problem in your environment. Looking at the manifests different users and groups are the owners of the upstream config vs. the rest of the config, which may be causing issues for you. |
Hello @3flex Sorry for the poor issue, being in a hurry yesterday and wanted to submit the PR I'm having issues both with vhosts and upstreams resources trying to be created before NGINX is even installed.
I can confirm that by adding the requires above the errors are solved, just check it again in a local env. It maybe related to how we use NGINX module in our manifests, but seems logical for me to require the base folder for both vhost and upstreams or to ensure NGINX is installed before creating resources. Digging in our manifests, we don't require NGINX to be installed before creating them, but this use to work as expected before. include dotnginx
$nginx = parseyaml(template('dotdrupal/nginx_vhost.yaml.erb'))
$vhost = $nginx['nginx::nginx_vhosts']
create_resources('nginx::resource::vhost', $vhost, {})
$locations = $nginx['nginx::nginx_locations']
create_resources('nginx::resource::location', merge($locations, hiera('dotdrupal::nginx::extra_locations', {})), {})
$upstream = $nginx['nginx::nginx_upstreams']
create_resources('nginx::resource::upstream', $upstream, {}) Regarding versions, i'm testing against latest commit from both https://github.com/jfryman/puppet-nginx and https://github.com/puppetlabs/puppetlabs-concat with puppet version 3.7.4 in a debian7 box |
Also puppet-nginx/#610 is having the same issue. |
Can you try puppetlabs-concat 1.2.1? In that version the standard file resource is used to create the upstream.conf file, and should autorequire the parent directory provided From what I can see this has been changed in concat master branch, now the custom type I just want to make sure this gets fixed in the right place. |
@3flex just tested it against latest nginx and puppetlabs-concat 1.2.1 and the issues reported don't happen. Let me know if you want me to perform any other test. Regards M |
It sounds like an issue with puppetlabs-concat. I've raised https://tickets.puppetlabs.com/browse/MODULES-2023 I'll keep this open until there's a resolution of that ticket. For now I'd suggest switching to the Forge version of the puppetlabs-concat module. |
Unfortunate that my ticket didn't result in any changes before 2.0.0 was released. @Tombar if you'd be so kind could you please check with concat 2.0.0 both with and without your patch, and let me know results? Thanks! |
@3flex give me a couple days more and I will report back my findings. Regards and thank for your help! M |
Using this repo gives me the following warnings:
But it does seem to work |
Hello @3flex, reporting back concat module at 2.0.x branch + nginx module at last commit = same errors as reported here. concat module at 2.0.x branch + nginx module with my PR = works + concat ensure parameter warning since my PR doesn't have this previous PR merged Sorry for my late response. Regards M |
Require base folder for resources
Just FYI, will revert this once puppetlabs/puppetlabs-concat#319 is included in a release to Forge. |
This commit updates the nginx submodule to the voxpupuli repo (https://github.com/jfryman/puppet-nginx.git is just a reference to the voxpupuli repo, so nothing should change here) and checks out to a commit before the one that reverted the hostfix from voxpupuli/puppet-nginx#624 This fixes the problem with nginx trying to create a vhost configuration before installing the nginx package. This is caused by concat not requiring the underlying nginx file hierarchy. This solution needs to be reverted when concat devs come up with a working implementation of the autorequire function in `lib/puppet/type/concat_file.rb`. The current solution from ba643617a2a03f0588a0ec6625824a707528d9f0 (L112-L115) is not working and the original approach: puppetlabs/puppetlabs-concat#319 has been abandoned at some point. For more details see the ticket: https://datacentred.atlassian.net/browse/PD-2567.
This commit updates the nginx submodule to the voxpupuli repo (https://github.com/jfryman/puppet-nginx.git is just a reference to the voxpupuli repo, so nothing should change here) and checks out to a commit before the one that reverted the hostfix from voxpupuli/puppet-nginx#624 This fixes the problem with nginx trying to create a vhost configuration before installing the nginx package. This is caused by concat not requiring the underlying nginx file hierarchy. This solution needs to be reverted when concat devs come up with a working implementation of the autorequire function in `lib/puppet/type/concat_file.rb`. The current solution from ba643617a2a03f0588a0ec6625824a707528d9f0 (L112-L115) is not working and the original approach: puppetlabs/puppetlabs-concat#319 has been abandoned at some point. For more details see the ticket: https://datacentred.atlassian.net/browse/PD-2567.
Require base folder for resources
Require base folder for resources
Hello James
Not sure if this issue is related to the changes in the concat module, but since it was updated we start having errors like:
==> single: Error: Could not set 'file' on ensure: No such file or directory - /etc/nginx/conf.d/drupal-upstream.conf20150506-4095-ep3q6b.lock
==> single: Error: Could not set 'file' on ensure: No such file or directory - /etc/nginx/conf.d/drupal-upstream.conf20150506-4095-ep3q6b.lock
==> single: Wrapped exception:
==> single: No such file or directory - /etc/nginx/conf.d/drupal-upstream.conf20150506-4095-ep3q6b.lock
==> single: Error: /Stage[main]/Dotdrupal/Dotdrupal::Site[drupal]/Dotdrupal::Nginx[nginx for drupal]/Nginx::Resource::Upstream[drupal]/Concat[/etc/nginx/conf.d/drupal-upstream.conf]/File[/etc/nginx/conf.d/drupal-upstream.conf]/ensure: change from absent to file failed: Could not set 'file' on ensure: No such file or directory - /etc/nginx/conf.d/drupal-upstream.conf20150506-4095-ep3q6b.lock
We weren't having this before :( i've dig into NGINX manifest and find a way to solve them by just requiring the destination folder in the concat resources.
Hope it can be merged
Regards
M