-
-
Notifications
You must be signed in to change notification settings - Fork 875
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
Use vhost_location_directory if its parameters are defined #199
Conversation
Since root can be defined at vhost level there isn't a need to define (www_)root in every location. Here I've added every parameter that has a default of undef and is used by vhost_location_directory.erb to the condition that uses the vhost_location_directory.erb file. As location_custom_cfg and location are only parameters used by the vhost_location_empty it makes sense to use vhost_location_directory.erb whenever possible. No compatibility issues are raised as all of the tested vhost_location_directory variables aren't used in the vhost_location_empty.erb template.
Add context to the nginx::resource::location error message to make it useful
fix test case for using vhost_location_directory (accepted parameters increase)
fix test case. parse error
ok. I'm really struggling to get this test passing travis. |
@@ -504,7 +504,7 @@ | |||
:vhost => 'vhost1', | |||
} end | |||
|
|||
it { expect { should contain_class('nginx::resource::location') }.to raise_error(Puppet::Error, /Cannot create a location reference without a www_root, proxy, location_alias, fastcgi, stub_status, or location_custom_cfg defined/) } | |||
it { expect { should contain_class('nginx::resource::location') }.to raise_error(Puppet::Error, /Cannot create a location rspec-test(rspec-test) of vhost vhost1 without a www_root, location_cfg_prepend, location_cfg_append, try_files, autoindex, auth_basic, auth_basic_user_file, proxy, location_alias, fastcgi, stub_status, or location_custom_cfg defined/) } |
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.
This confused me as well, then I realized the new string to match has brackets which need to be escaped since it's a regex. So change this line to:
it { expect { should contain_class('nginx::resource::location') }.to raise_error(Puppet::Error, /Cannot create a location rspec-test\(rspec-test\) of vhost vhost1 without a www_root, location_cfg_prepend, location_cfg_append, try_files, autoindex, auth_basic, auth_basic_user_file, proxy, location_alias, fastcgi, stub_status, or location_custom_cfg defined/) }
The tests were broken by merging #203 which is why the tests for this PR aren't green - they need updating to reflect that change. |
Yep. I'm still trying to work out how to do a standalone test without depending on the magic of Travis. |
Way old. Thanks for taking a pass at this... going to revisit when 🕐 allows. Please reopen and rebase if you wanna take another pass at this. |
Since root can be defined at vhost level there isn't a need to define (www_)root in every location.
Here I've added every parameter that has a default of undef and is used by vhost_location_directory.erb to the condition that uses the vhost_location_directory.erb file.
As location_custom_cfg and location are only parameters used by the vhost_location_empty it makes sense to use vhost_location_directory.erb whenever possible.
No compatibility issues are raised as all of the tested vhost_location_directory variables aren't used in the vhost_location_empty.erb template.