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

[Modules 5519] Add port parameter in class mod::jk #1679

Merged
merged 21 commits into from Sep 1, 2017

Conversation

EmersonPrado
Copy link
Contributor

New parameters allow changing binding settings for mod_jk. This way, one can use Apache with mod_jk and a reverse proxy / cache receiving requests and forwarding to Apache in a different port, for example.

@EmersonPrado
Copy link
Contributor Author

Finally, a working spec test with probably enough contexts

@@ -12,13 +12,19 @@
it { is_expected.to contain_file('jk.conf').that_notifies('Class[apache::service]') }
end

context "RHEL 6 with only required facts and no parameters" do
default_ip = '192.168.1.1'
Copy link
Contributor

Choose a reason for hiding this comment

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

as I mentioned in #1671, let statements should be used in these cases, e.g.:

let (:default_ip) { '192.168.1.1' }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had a problem in #1671 regarding this change. I'll wait 'till we solve that then replicate the solution here.

@@ -2165,7 +2166,31 @@ class { '::apache::mod::jk':

**Parameters within `apache::mod::jk`**:

The best source for understanding the `mod_jk` parameters is the [official documentation](https://tomcat.apache.org/connectors-doc/reference/apache.html), except for \*file_content:
The best source for understanding the `mod_jk` parameters is the [official documentation](https://tomcat.apache.org/connectors-doc/reference/apache.html), except for:
Copy link
Contributor

Choose a reason for hiding this comment

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

yes! we like linking to official docs!

Optional[String] $ip = undef,
Pattern[/^\d+$/] $port = '80',
Boolean $add_listen = true,
# Conf file content
$workers_file = undef,
Copy link
Contributor

Choose a reason for hiding this comment

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

please align the rest of these =

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@@ -9,6 +9,11 @@
# https://tomcat.apache.org/connectors-doc/reference/apache.html
#
class apache::mod::jk (
# Binding to mod_jk
Optional[String] $ip = undef,
Pattern[/^\d+$/] $port = '80',
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a particular reason you're not using the Integer type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The variable $port is used as a string - interpolated in another string in a ensure_resource declaration

@eputnam
Copy link
Contributor

eputnam commented Aug 28, 2017

@EmersonPrado could you please rebase this on master? basically just adding this work to your previous PR for paths.

@EmersonPrado
Copy link
Contributor Author

I rebased and pushed again, but it didn't come out right:

 ! [rejected]        MODULES-5519 -> MODULES-5519 (non-fast-forward)
error: failed to push some refs to 'https://github.com/EmersonPrado/puppetlabs-apache.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Integrate the remote changes (e.g.
hint: 'git pull ...') before pushing again.

And now the PR has an unsolved conflict. I did solve all conflicts locally while rebasing, so I believe to have pushed a clean tree. Do you have a hint? It's the first time I rebase something I have already pushed (in fact, I always thought I shouldn't), so I don't really know how to solve this.

@EmersonPrado
Copy link
Contributor Author

I think I could solve all conflicts manually from the Web interface. Are we done?

@ekohl
Copy link
Collaborator

ekohl commented Aug 29, 2017

@EmersonPrado you have to force push (git push -f) after a rebase. Since you've rewritten the history, it's no longer fast-forward. Doing non-fast-forward pushes on shared branches (master, release branches) is bad, hence the warning, but on PR branches it's fine.

@eputnam eputnam merged commit dc85ed2 into puppetlabs:master Sep 1, 2017
cegeka-jenkins pushed a commit to cegeka/puppet-apache that referenced this pull request Jul 15, 2020
* Add ip and port parameters to class apache::mod::jk

* Add parameter to make binding conditional in class apache::mod::jk

* Add binding parameters validation in apache::mod::jk

* Add default binding params in mod::jk spec test

* Add context with alternative binding port in mod::jk spec test

* Add context without binding in mod::jk spec test

* Add in README binding parameters info for class mod::jk

* Add in README use cases for apache::mod::jk binding params

* Change default param treatment in mod::jk to allow spec test

* Correct parameters in mod::jk spec test

* Use variable $facts for ip param default

* Use facts for param defaults in mod::jk spec test

* Correct facts hash reference in class mod::jk

* Change param default treatment to allow spec test in class mod::jk

* Add resource count in all contexts in mod::jk spec test

* Change resource count for specific resource checks in mod::jk spec test

* Add context for supplied IP param in mod::jk spec test

* Align parameter values in class mod::jk

* Change param apache::mod::jk::port type to integer

* Change param apache::mod::jk::port type to integer in spec test
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

3 participants