MODULES-5426 : Add support for all mod_passenger server config settings #1665

dacat · 2017-08-06T17:27:05Z

Support for the available server configuration settings is currently incomplete. The changes I've will:

Add support for all currently documented server config settings
Maintain support for the current set of apache::mod::passenger settings
Check if the user provided version of passenger supports, deprecated or removed the server config setting, Note: only works if passenger_installed_version is set.
Add basic rspec tests for the changes
Add basic acceptances tests to validate the support, deprecated, and removed functionality

Items from the guidlines for contributing:

Acceptance tests pass
Rspec tests pass
Documentation updated

* Parsed https://www.phusionpassenger.com/library/config/apache/reference for all options * Took care of maintaining older options * Add specs for all options * added warnings a failures if passenger_installed_version is set

* Remove the RubyMine formatter configuration from the ERB template * Remove pending message from spec/acceptance/mod_passenger_spec The message is not needed any more because puppet manages the passenger.conf file now

* validates the passenger_installed_version options fires the version checking for each option * Fix a few linting errors

The code generator was using the introduction version number as the removed verison number.

dacat · 2017-08-07T00:36:53Z

I am finished making changes to this branch.

HAIL9000 · 2017-08-16T18:13:05Z

Hey @jbondpdx, would you mind taking a looking at these README changes and seeing what you think? They don't match the standard format we usually use for parameters but seem like an improvement with respect to readability and not duplicating documentation.

hunner · 2017-08-16T18:43:39Z

manifests/mod/passenger.pp

+      }
+    }
+    if $passenger_app_root {
+      if (versioncmp($passenger_installed_version, '4.0.0') < 0) {


Would it be easier to sort the parameters based on their version instead of duplicating the versioncmp logic a bunch?

if (versioncmp($passenger_installed_version, '4.0.0') < 0) { if $passenger_allow_encoded_slashes { fail("Passenger config option :: passenger_allow_encoded_slashes is not introduced until version 4.0.0 :: ${passenger_installed_version} is the version reported") } if $passenger_app_env { fail("Passenger config option :: passenger_app_env is not introduced until version 4.0.0 :: ${passenger_installed_version} is the version reported") } if $passenger_app_group_name { fail("Passenger config option :: passenger_app_group_name is not introduced until version 4.0.0 :: ${passenger_installed_version} is the version reported") } if $passenger_app_root { fail("Passenger config option :: passenger_app_root is not introduced until version 4.0.0 :: ${passenger_installed_version} is the version reported") } }

You are right, I should of lumped together the checks. I thought about but I rationalized to keep it the way it was written -- why? I don't recall. I will fix.

I do recall now why it its that way. It was to keep all the passenger options in alphabetical order. Keep it as is, or lump them together?

Normally we keep these alphabetized just to make things easier to find. If sorting by version in this case works better, then I'm fine with it.

hunner · 2017-08-16T18:44:39Z

spec/classes/mod/passenger_spec.rb

+            :concat_basedir         => '/dne',
+            :id                     => 'root',
+            :path                   => '/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin',
+            :is_pe                  => false,


Why indent extra here?

hunner · 2017-08-16T18:44:51Z

spec/classes/mod/passenger_spec.rb

+            :concat_basedir         => '/dne',
+            :id                     => 'root',
+            :path                   => '/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin',
+            :is_pe                  => false,


hunner · 2017-08-16T18:44:58Z

spec/classes/mod/passenger_spec.rb

+            :concat_basedir         => '/dne',
+            :id                     => 'root',
+            :path                   => '/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin',
+            :is_pe                  => false,


hunner · 2017-08-16T18:45:14Z

spec/classes/mod/passenger_spec.rb

+            :concat_basedir         => '/dne',
+            :id                     => 'root',
+            :path                   => '/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin',
+            :is_pe                  => false,


hunner · 2017-08-16T18:45:29Z

spec/classes/mod/passenger_spec.rb

+          :id                     => 'root',
+          :kernel                 => 'Linux',
+          :path                   => '/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin',
+          :is_pe                  => false,


hunner · 2017-08-16T18:45:54Z

spec/classes/mod/passenger_spec.rb

+          :id                     => 'root',
+          :kernel                 => 'FreeBSD',
+          :path                   => '/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin',
+          :is_pe                  => false,


hunner · 2017-08-16T18:46:04Z

spec/classes/mod/passenger_spec.rb

+          :id                     => 'root',
+          :kernel                 => 'Linux',
+          :path                   => '/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/opt/bin',
+          :is_pe                  => false,


hunner · 2017-08-16T18:47:55Z

spec/classes/mod/passenger_spec.rb

@@ -25,7 +204,7 @@
    it { is_expected.to contain_file('passenger.conf').with({
      'path' => '/etc/apache2/mods-available/passenger.conf',
    }) }
-    describe "with passenger_root => '/usr/lib/example'" do
+     describe "with passenger_root => '/usr/lib/example'" do


Whitespace change.

All white space issues are either copy/paste or code generation issues. I will fix.

eputnam · 2017-08-16T18:58:57Z

spec/classes/mod/passenger_spec.rb

+                                                            }) }
+
+    passenger_config_options = {
+        'passenger_allow_encoded_slashes' => {Type: 'OnOff', PassOpt: :PassengerAllowEncodedSlashes},


It looks like you're using capitalized key names whereas in ruby this is not canonical.

e.g. Type would be type, PassOpt would be pass_opt

Also, I'm curious, is there a particular reason the values for PassOpt are symbols instead of strings?

No reason :) Just generated code, I will make it more canonical.

eputnam · 2017-08-16T19:01:42Z

templates/mod/passenger.conf.erb

+  PassengerErrorOverride <%= @passenger_error_override %>
+  <%- end -%>
+  <%- if @passenger_file_descriptor_log_file -%>
+  PassengerFileDescriptorLogFile "<%= @passenger_file_descriptor_log_file %>"


I don't quite understand why some values get quotes and some don't. Could you expand on this decision?

Some of the values may have white space. The passenger docs is not quote explicit on which ones can and can't. The passenger docs specify in the syntax the kind of value and from there I wrote a conversion chart. Copy-paste from my script below:

type_conversion_chart = { 'dir-path' => 'FullPath', 'file-path' => 'FullPath', 'on|off' => 'OnOff', 'uri' => 'URI', 'path-to-ruby-interpreter' => 'FullPath', 'path-to-python-interpreter' => 'FullPath', 'path-to-node-js' => 'FullPath', 'path-to-json-settings-file' => 'FullPath', 'name' => 'String', 'path' => 'FullPath', 'relative-path' => 'RelPath', 'absolute-or-relative-path' => 'Path', 'smart|direct' => %w(smart direct), 'username' => 'String', 'groupname' => 'String', 'scheme://user:password@proxy_host:proxy_port' => 'URI', 'integer' => 'Integer', 'seconds' => 'Integer', 'number' => 'Integer', 'process|thread' => %w(process thread), 'megabytes' => 'Integer', 'url' => 'URI', 'bytes' => 'Integer', 'filename' => 'FullPath', 'path-to-socket' => 'FullPath', 'your_application_key' => 'String', '"string"' => 'QuotedString', 'gateway_hostname' => 'URI', 'gateway_port' => 'Integer', 'gateway_pinned_certificate_file' => 'FullPath' }

Here is the bit where at I decided what was what.
quoted_types = ['QuotedString', 'FullPath', 'Path', 'RelPath']
non_quoted_types = ['OnOff','Integer','String','URI']

Things I mapped as non_quoted_types are the only settings in which the passenger documentation made evident the value would not contain white space, everything else may contain white space.

The apache documetation states

Arguments to directives are separated by whitespace. If an argument contains spaces, you must enclose that argument in quotes.

I also left in whatever quoting existed before my changes. I didn't want to change anyone's module configuration who was using existing options with or without quotes.

HAIL9000 · 2017-08-16T20:28:03Z

spec/classes/mod/passenger_spec.rb

+    passenger_config_options.each do |config_option, config_hash|
+      puppetized_config_option = config_option
+      valid_config_values = []
+      case config_hash[:Type]


I'm curious about the sorting of these tests into cases by the Type. Is this just a semantic thing to make the test cases more organized or does it serve a functional purpose?

During the code generator work, I had a different case for each "type" then slowly consolidated. When I saw that some of the existing apache::mod::passenger options had types associated with them, ex. $passenger_log_file and $passenger_spawn_method, my thought was to lay down some ground to maybe force type checking and allow unhappy-path testing. its probably too early for that, i'll will consolidate and remove duplicate code.

jbondpdx

Awesome change, thank you!

jbondpdx · 2017-08-16T20:53:46Z

README.md

-  Default: `undef`.
-
-* `passenger_max_pool_size`: Sets the [`PassengerMaxPoolSize`](https://www.phusionpassenger.com/library/config/apache/reference/#passengermaxpoolsize).
+The current set of server configurations settings were taking directly from the [`Passenger Reference`](https://www.phusionpassenger.com/library/config/apache/reference/). Deprecation warning and removal failure messages can be enabled by setting the passenger_installed_version to


Suggest changing the first sentence to "These server configuration settings are based on the Passenger Reference". Passenger Reference doesn't need to be in backticks.

Also, passenger_installed_version is a parameter, so should be in backticks.

jbondpdx · 2017-08-16T20:54:52Z

README.md

+|union_station_key|undef|[`UnionStationKey`](https://www.phusionpassenger.com/library/config/apache/reference/#UnionStationKey)||
+|union_station_proxy_address|undef|[`UnionStationProxyAddress`](https://www.phusionpassenger.com/library/config/apache/reference/#UnionStationProxyAddress)||
+|union_station_support|undef|[`UnionStationSupport`](https://www.phusionpassenger.com/library/config/apache/reference/#UnionStationSupport)||
+|wsgi_auto_detect|undef|WsgiAutoDetect|These options have been removed in version 4.0.0 as part of an optimization. You should use PassengerEnabled instead.|


holy cow, this table is a heroic effort, go you! Huge improvement.

dacat · 2017-08-19T17:30:30Z

@hunner @HAIL9000 @jbondpdx @eputnam My apologies for not getting back to you sooner, I was out of the country. About 90% of the code and documentation is being generated by a script I wrote. This effort started as just adding support for the server config settings. It sort of snowballed as I kept adding to the code generator. I will fix the issues with the generator and formatting as you have pointed out. However, before I left I started on a branch to combine all the changes for Vhost, Directory and Server configurations. With that I have a few questions.

Would you prefer I decline these pull requests and request one that adds all the features combining https://tickets.puppetlabs.com/browse/MODULES-5426 https://tickets.puppetlabs.com/browse/MODULES-5428 and https://tickets.puppetlabs.com/browse/MODULES-5430?
However you prefer is fine with me.
In regards to documentation, edits I made to my code/doc generator which adds in which apache directives the passenger options are available. This adds another column to the table. Is that acceptable or do you prefer something else?

Thanks for taking the time to review my pull request.

eputnam · 2017-08-21T18:25:10Z

@dacat keeping the PRs separate works best. And I don't mind an extra column but I'm not sure I understand what it would be for.

* Remove whitespace added by auto-formatter * Make hash keys more canonical * Remove UnionStation options - UnionStation is no longer supported/offered by phusion. - All options are being removed from the passenger source * Add white space at the end of the the passenger.conf.erb template * Update the README to remove UnionStation as well as to add the context column.

dacat · 2017-08-21T21:52:00Z

@eputnam I added the column in the README for you to look at. All of the passenger options are available in the context of a "server config". From there subsets are available in "vhost", "directory" and "htaccess" Since all the passenger options are available, as of today, in the "server config" context, it made sense in my head to list the other context's which can contain the option.

jbondpdx

Just this one little fix needed.

jbondpdx · 2017-09-06T21:39:54Z

README.md

-  Default: `undef`.
-
-* `passenger_max_pool_size`: Sets the [`PassengerMaxPoolSize`](https://www.phusionpassenger.com/library/config/apache/reference/#passengermaxpoolsize).
+The current set of server configurations settings were taking directly from the [Passenger Reference](https://www.phusionpassenger.com/library/config/apache/reference/). Deprecation warning and removal failure messages can be enabled by setting the `passenger_installed_version` to


Change "taking" to "taken"

Optional bonus points if you make the "Deprecation warnings..." sentence active while you're in there: "To enable deprecation warnings and removal failure messages, set the..."

dacat · 2017-09-29T15:18:33Z

My apologies for the delay.

I've verified requested changes have been made, Jean is real busy so we'll just check this off for her

…tions MODULES-5426 : Add support for all mod_passenger server config settings

dacat added 7 commits August 5, 2017 18:38

Fix accpetance test error for mod_passenger

5a9717f

* Remove the RubyMine formatter configuration from the ERB template * Remove pending message from spec/acceptance/mod_passenger_spec The message is not needed any more because puppet manages the passenger.conf file now

Add acceptance test for mod_passenger options

bc13cb0

* validates the passenger_installed_version options fires the version checking for each option * Fix a few linting errors

MODULES-5426 : Fix bug for REMOVED options

267e0a8

The code generator was using the introduction version number as the removed verison number.

MODULES-5426 : Update comments in the list of template vars

015263a

MODULES-5426 : Update the README.md mod_passenger documentation

39952b1

MODULES-5426 : Fix error introduced during refactor

a902627

hunner reviewed Aug 16, 2017

View reviewed changes

eputnam reviewed Aug 16, 2017

View reviewed changes

HAIL9000 reviewed Aug 16, 2017

View reviewed changes

jbondpdx previously requested changes Aug 16, 2017

View reviewed changes

HAIL9000 mentioned this pull request Aug 16, 2017

MODULES-5428 : Add passenger setting support inside Directory directive #1666

Closed

dacat added 2 commits August 21, 2017 16:44

Merge branch 'master' into update_passenger_mod_options

9829dc2

jbondpdx reviewed Sep 6, 2017

View reviewed changes

MODULES-5426 : Minor changes to the readme

26be410

HAIL9000 merged commit 12b366c into puppetlabs:master Oct 5, 2017

cegeka-jenkins pushed a commit to cegeka/puppet-apache that referenced this pull request Jul 15, 2020

Merge pull request puppetlabs#1665 from dacat/update_passenger_mod_op…

56c9ae5

…tions MODULES-5426 : Add support for all mod_passenger server config settings

GSPatton added the feature label Jun 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MODULES-5426 : Add support for all mod_passenger server config settings #1665

MODULES-5426 : Add support for all mod_passenger server config settings #1665

dacat commented Aug 6, 2017 •

edited

dacat commented Aug 7, 2017

HAIL9000 commented Aug 16, 2017

hunner Aug 16, 2017

dacat Aug 19, 2017 •

edited

dacat Aug 21, 2017

jbondpdx Sep 29, 2017

hunner Aug 16, 2017

hunner Aug 16, 2017

hunner Aug 16, 2017

hunner Aug 16, 2017

hunner Aug 16, 2017

hunner Aug 16, 2017

hunner Aug 16, 2017

hunner Aug 16, 2017

dacat Aug 19, 2017

eputnam Aug 16, 2017 •

edited

dacat Aug 19, 2017

eputnam Aug 16, 2017

dacat Aug 19, 2017

HAIL9000 Aug 16, 2017

dacat Aug 20, 2017

jbondpdx left a comment

jbondpdx Aug 16, 2017

jbondpdx Aug 16, 2017

dacat commented Aug 19, 2017 •

edited

eputnam commented Aug 21, 2017

dacat commented Aug 21, 2017

jbondpdx left a comment

jbondpdx Sep 6, 2017

dacat commented Sep 29, 2017

MODULES-5426 : Add support for all mod_passenger server config settings #1665

MODULES-5426 : Add support for all mod_passenger server config settings #1665

Conversation

dacat commented Aug 6, 2017 • edited

dacat commented Aug 7, 2017

HAIL9000 commented Aug 16, 2017

Choose a reason for hiding this comment

dacat Aug 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eputnam Aug 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbondpdx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dacat commented Aug 19, 2017 • edited

eputnam commented Aug 21, 2017

dacat commented Aug 21, 2017

jbondpdx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dacat commented Sep 29, 2017

dacat commented Aug 6, 2017 •

edited

dacat Aug 19, 2017 •

edited

eputnam Aug 16, 2017 •

edited

dacat commented Aug 19, 2017 •

edited