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-1478: Add a $purge_connectors parameter. #56

Merged
merged 1 commit into from Nov 10, 2014

Conversation

philipwigg
Copy link
Contributor

See MODULES-1478.

Basically the way that this module adds additional Connector elements seems very un-Puppet-like.

This PR adds a parameter called $purge_connectors which removes all existing connectors for a given protocol so that if you define a single HTTP connector on port 8140 then that's the only HTTP connector you will have after the module has been applied. I think this is what most users will expect and want.

I've set it to false by default so that it's not a breaking change.

}

if $purge_connectors and ($connector_ensure =~ /^(absent|false)$/) {
fail('$connector_ensure must be set to \'true\' or \'present\' to use $purge_connections')
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be $purge_connectors not $purge_connections

@underscorgan
Copy link
Contributor

@philipwigg other than the one fail message comment, so many 👍s for this. so many.

@philipwigg
Copy link
Contributor Author

I've fixed the fail message and added a test for it.

Thanks for reviewing my PR!

@@ -228,6 +228,10 @@ Sets the group to run Tomcat as.

Specifies whether or not to install from source. A Boolean that defaults to 'true'.

#####`$purge_connectors`

Specifies whether or not to purge existing Connector elements from server.xml. For example, if hyou define an AJP connector using tomcat::instance::connector, then existing AJP connectors will be purged beforehand if $purge_connections is set to 'true'. Otherwise by default a new AJP connector element would be created which might not be what you expect. This makes defining connectors work in a declaritve way. A Boolean that defaults to 'false'.
Copy link
Contributor

Choose a reason for hiding this comment

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

The second sentence is a little confusing, maybe reword to:
For example, if you define an AJP connector using tomcat::instance::connector and $purge_connectors is set to true, existing AJP connectors will be removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

declarative is misspelled.

@underscorgan
Copy link
Contributor

@philipwigg ok, just found a couple of other cleanup things in the README. Once you've fixed those could you please rebase and squash to a single commit? Thanks!

@philipwigg
Copy link
Contributor Author

@mhaskel I rewrote the README a bit more, rebased and squashed. Thanks for your input!

underscorgan pushed a commit that referenced this pull request Nov 10, 2014
MODULES-1478: Add a $purge_connectors parameter.
@underscorgan underscorgan merged commit 3ae328e into puppetlabs:master Nov 10, 2014
@underscorgan
Copy link
Contributor

Thanks for the contribution @philipwigg

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