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.

Sign up for GitHub

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

Jump to bottom

Convert sendfile param to string from bool. #365

Merged
merged 1 commit into from
Sep 22, 2013
Merged

Convert sendfile param to string from bool. #365

merged 1 commit into from
Sep 22, 2013

Conversation

razorsedge
Copy link
Contributor

Currently, one would assume that since the default is false, setting sendfile => true would cause the option to be enabled in httpd.conf, but instead it writes EnableSendfile true which is incorrect. This makes the usage consistant with commit a82af55.

@blkperl
Copy link
Contributor

blkperl commented Sep 19, 2013

The docs seem to imply that sendfile is on by default. Wouldn't this be changing the module default to enforce that it is off by default.

http://httpd.apache.org/docs/2.2/mod/core.html#enablesendfile

@razorsedge
Copy link
Contributor Author

I was assuming that the module default was false and thus EnableSendfile Off in httpd.conf, but the ERB is broken such that the sendfile line is actually missing and thus set to the compiled-in default. I will change the PR to result in EnableSendfile On by default.

@blkperl
Copy link
Contributor

blkperl commented Sep 19, 2013

Sweet, my only other concern then is EnableSendFile On the default for both RHEL and Debian if not then we should put it in the params.pp class?

@razorsedge
Copy link
Contributor Author

Updated PR and added rspec tests.

blkperl
blkperl reviewed Sep 20, 2013
View reviewed changes
spec/classes/apache_spec.rb
@@ -288,5 +288,30 @@
it { should contain_class('apache::mod::mime') }
end
end

describe "sendfile" do
context "foo" do
Copy link
Contributor

Choose a reason for hiding this comment

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

For clarity this should probably be "with invalid value"

@razorsedge
Copy link
Contributor Author

Updated PR to fix rspec context name.

@blkperl
Copy link
Contributor

blkperl commented Sep 21, 2013

Just checked the README, and it says sendfile defaults to false, can you fix that too?

@razorsedge
Convert sendfile param to string from bool.
f349262 
Currently, one would assume that since the default is false, setting `sendfile
=> true` would cause the option to be enabled in httpd.conf, but instead it
writes `sendfile true` which is incorrect.  This makes the usage consistant
with commit a82af55.  Includes rspec
tests and input validation.
@razorsedge
Copy link
Contributor Author

@blkperl Good catch. I always forget the README.

blkperl added a commit that referenced this pull request Sep 22, 2013
@blkperl
Merge pull request #365 from razorsedge/sendfile
7bc5170 
Convert sendfile param to string from bool.
@blkperl blkperl merged commit 7bc5170 into puppetlabs:master Sep 22, 2013
@blkperl
Copy link
Contributor

blkperl commented Sep 22, 2013

Thanks for the contribution!

traylenator pushed a commit to traylenator/puppetlabs-apache that referenced this pull request Jun 7, 2022
@lowerpuppet @davejrt
Fixing incorrect variable names in docker_compose/ruby.rb (puppetlabs…
059fe18 
…#365)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@razorsedge @blkperl @GSPatton