-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[Modules 5492] - Include treatment for absolute, relative and pipe paths for JkLogFile and JkShmFile for class mod::jk #1671
Conversation
@EmersonPrado, good to see you working on apache again! Would you mind adding some unit tests for this and update the README to document/explain the new defaults and how those params now work? |
@eputnam Sure! We're using this module extensively at work, so we're really willing to improve as much as we can. |
Revert "Try using symbol to test conf file in mod::jk spec test" This reverts commit b02c31d. Revert "Set mod_dir variable in mod::jk spec test" This reverts commit 9f7e941. Revert "Create mod_dir variable in mod::jk class to ease spec test" This reverts commit 39986b8. Revert "Remove content check for jk.conf in mod::jk spec test" This reverts commit d85e8a2. Revert "Include pipe paths context in mod::jk spec test" This reverts commit 95dd58b. Revert "Include absolute paths context in mod::jk spec test" This reverts commit 70e36e2. Revert "Include relative paths context in mod::jk spec test" This reverts commit 68c15ae. Revert "Include conf file check for new contexts in mod::jk spec test" This reverts commit 45d040c. Revert "Include context loop with one item in mod::jk spec test" This reverts commit f6bb97f.
OK, I have to admit I know squat about spec tests. I keep hoping its documentation improves to the point I can really solve this kind of stuff. Gave up for today. |
spec/classes/mod/jk_spec.rb
Outdated
@@ -115,7 +115,7 @@ | |||
|
|||
it_behaves_like 'minimal resources' | |||
it do | |||
is_expected.to contain_file("/etc/httpd/conf.d/jk.conf") | |||
is_expected.to contain_file("#{:mod_dir}/jk.conf") |
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 is a bit of a gotcha. contain_file
is referencing the file resource in the catalog by its title. If you look in jk.pp you'll see that the config file has a hard-coded title of 'jk.conf' which is why this test wasn't working. I think what you're looking for is something more like this:
is_expected.to contain_file('jk.conf').with({ :path => "#{mod_dir}/jk.conf" })
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.
Ah, that should be it! I was using the path, when I should be using the resource title. I'll start over pushing very little at a time, so we can get somewhere. Thanks for the hint!
@eputnam Now we've got updated docs and functional tests covering all situations envolved. Anything else comes to mind? |
spec/classes/mod/jk_spec.rb
Outdated
it { | ||
verify_contents(catalogue, 'jk.conf', ['<IfModule jk_module>', '</IfModule>']) | ||
} | ||
|
||
end | ||
|
||
shm_log_paths = { |
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 should/can be a let statement as well:
let (:shm_log_paths) do
{ ... }
end
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.
rspec-puppet didn't like the change:
https://travis-ci.org/puppetlabs/puppetlabs-apache/jobs/268018580#L799
https://travis-ci.org/puppetlabs/puppetlabs-apache/jobs/268018581#L680
'shm_log_paths
is not available on an example group (e.g. a describe
or context
block). It is only available from within individual examples (e.g. it
blocks) or from constructs that run in the scope of an example (e.g. before
, let
, etc).'
What is the correct way to reference local variables declared in let
statements? Current form is this
I left one more tiny comment regarding some rspec-fu and then we'll get this merged |
oh i see what happened. i'm sorry, i guess i was wrong. what you can do is call |
…ths for JkLogFile and JkShmFile for class mod::jk (puppetlabs#1671) * Move logroot treatment away from files treatment in mod::jk * Treat Log/Shm files with absolute paths in mod::jk * Add default case for Log/Shm files in mod::jk * Include in README subsection on ...mod::jk::logroot parameter * Include in README subsection on ...mod::jk::(shm|log)_file parameters * Include in README examples on ...mod::jk::(shm|log)_file parameters * Include context loop with one item in mod::jk spec test * Include conf file check for new contexts in mod::jk spec test * Include relative paths context in mod::jk spec test * Include absolute paths context in mod::jk spec test * Include pipe paths context in mod::jk spec test * Remove content check for jk.conf in mod::jk spec test * Create mod_dir variable in mod::jk class to ease spec test * Set mod_dir variable in mod::jk spec test * Try using symbol to test conf file in mod::jk spec test * Reverts all failed attempts to spec test jk.conf Revert "Try using symbol to test conf file in mod::jk spec test" This reverts commit b02c31d. Revert "Set mod_dir variable in mod::jk spec test" This reverts commit 9f7e941. Revert "Create mod_dir variable in mod::jk class to ease spec test" This reverts commit 39986b8. Revert "Remove content check for jk.conf in mod::jk spec test" This reverts commit d85e8a2. Revert "Include pipe paths context in mod::jk spec test" This reverts commit 95dd58b. Revert "Include absolute paths context in mod::jk spec test" This reverts commit 70e36e2. Revert "Include relative paths context in mod::jk spec test" This reverts commit 68c15ae. Revert "Include conf file check for new contexts in mod::jk spec test" This reverts commit 45d040c. Revert "Include context loop with one item in mod::jk spec test" This reverts commit f6bb97f. * Add mod_dir variable in mod::apache class to allow spec tests * Add mod_dir variable in mod::apache spec test * Correct mod_dir variable reference in class mod::jk * Add parameter in shared example of mod::jk spec test * Include path check for jk.conf in mod::jk spec test * Correct Debian path for jk.conf in mod::jk spec test * Compound matchers for same resource in mod::jk spec test * Include "and" in additional matcher for jk.conf in mod::jk spec test * Try to correct compound matchers syntax in mod::jk spec test * Remove compound matchers in mod::jk spec test * Move simple matcher to single line in mod::jk spec test * Include jk.conf check via "with_content" in mod::jk spec test * Correct jk.conf contents in mod::jk spec test * Correct line endings in jk.conf contents in mod::jk spec test * Create hash for (shm|log)_file parameters in mod::jk spec test * Add context iterated for (shm|log)_file in mod::jk spec test * Include iterated file content check in mod::jk spec test * Remove redundant jk.conf content check in mod::jk spec test * Add relative paths for (shm|log)_file in mod::jk spec test * Remove remaining redundant jk.conf content check in mod::jk spec test * Add absolute paths for (shm|log)_file in mod::jk spec test * Add pipe paths for (shm|log)_file in mod::jk spec test * Correct log_file pipe parameter in mod::jk spec test * Use "let" to set local variable in mod::jk spec test * Change symbol to var name in assignment in mod::jk spec test * Quote var name in assignment in mod::jk spec test * Correct symbol reference in mod::jk spec test * Change unecessary var for direct hash iteration in mod::jk spec test
Previous version prepended logdir in any filename, resulting in incorrect paths for absolute and pipe paths.