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 fcgid options #716

Merged
merged 1 commit into from
Jun 3, 2014
Merged

Add fcgid options #716

merged 1 commit into from
Jun 3, 2014

Conversation

ekohl
Copy link
Collaborator

@ekohl ekohl commented May 2, 2014

This adds fcgid.conf to configure the fcgid module. It also adds the
FcgidWrapper option to vhost's directories.

The vhost option lacks any validation and allows users to shoot themselves in
the foot, but without iteration from the puppet future parser I don't see a
way to add the validation. Iteration would also allow auto including
apache::mod::fcgid.

@ekohl
Copy link
Collaborator Author

ekohl commented May 2, 2014

Failure from Travis:

/home/travis/build/puppetlabs/puppetlabs-apache/spec/classes/apache_spec.rb:154: warning: regexp has invalid interval
/home/travis/build/puppetlabs/puppetlabs-apache/spec/classes/apache_spec.rb:154: warning: regexp has `}' without escape
/home/travis/build/puppetlabs/puppetlabs-apache/spec/classes/apache_spec.rb:154: warning: regexp has invalid interval
/home/travis/build/puppetlabs/puppetlabs-apache/spec/classes/apache_spec.rb:154: warning: regexp has `}' without escape
/home/travis/.rvm/gems/ree-1.8.7-2012.02/gems/json-1.8.1/lib/json/ext/parser.so: [BUG] Segmentation fault
ruby 1.8.7 (2013-06-27 MBARI 8/0x6770 on patchlevel 374) [x86_64-linux], MBARI 0x6770, Ruby Enterprise Edition 2012.02

I think this is unrelated to my change. In puppet-foreman we set the 1.8.7 ruby explicitly to MRI in theforeman/puppet-foreman@93fb9b4 to avoid REE.

@igalic
Copy link
Contributor

igalic commented May 18, 2014

Thank you very much for your contribution, @ekohl!
The only thing this is missing is an update to the README.

@ekohl
Copy link
Collaborator Author

ekohl commented May 19, 2014

@igalic Added some documentation in the README.

@apenney
Copy link
Contributor

apenney commented May 22, 2014

This looks great! The only thing missing is the lack of a beaker acceptance test. Could you take a look in spec/acceptance and copy and tweak one of the existing tests to cover this? Thanks!

@igalic
Copy link
Contributor

igalic commented May 23, 2014

heheh...

._.

I hope that when @ekohl adds a new commit there won't be a third contributor coming along, saying,

"This looks fantastic, the only thing that's missing now …"

@ekohl
Copy link
Collaborator Author

ekohl commented May 23, 2014

Updated. @igalic Let's see if someone now says 'I like these RH tests, but where are the debian ones'.

This adds fcgid.conf to configure the fcgid module. It also adds the
FcgidWrapper option to vhost's directories.

The vhost option lacks any validation and allows users to shoot
themselves in the foot, but without iteration from the puppet future
parser I don't see a way to add the validation. Iteration would also
allow auto including apache::mod::fcgid.
@igalic
Copy link
Contributor

igalic commented May 26, 2014

@ekohl I guess if someone wants to go through the pain of supporting all platforms, they are welcome to submit their own pr.


A general observation here:

A puppet module is supposed to abstract away the need to consider differences between platforms.

A puppet module test then undoes all that process, by exposing all the guts, and being retracing all the steps.

I'm not sure we're doing testing right.

@ekohl
Copy link
Collaborator Author

ekohl commented Jun 1, 2014

@igalic I think tests can be written to share some things. Maybe it would be better to have a switch statement per testcase if there's platform specifics (httpd must be running vs apache2). Doing a curl to localhost could be shared since that part is platform independent. In the end you must test the guts here because that's what you are abstracting away from the user, but not every test is platform specific.

Does that make sense?

@igalic
Copy link
Contributor

igalic commented Jun 3, 2014

heh, yeah, does. Thanks for the sliver of hope ;)

igalic added a commit that referenced this pull request Jun 3, 2014
@igalic igalic merged commit b02d29e into puppetlabs:master Jun 3, 2014
@ekohl ekohl deleted the fcgid branch June 3, 2014 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants