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 support for SSLPassPhraseDialog #938

Merged
merged 1 commit into from
Dec 30, 2014
Merged

Add support for SSLPassPhraseDialog #938

merged 1 commit into from
Dec 30, 2014

Conversation

dteirney
Copy link

Initial PR for review to add support for SSLPassPhraseDialog.

https://tickets.puppetlabs.com/browse/MODULES-1518

@@ -7,7 +7,11 @@
AddType application/x-x509-ca-cert .crt
AddType application/x-pkcs7-crl .crl

<% if @ssl_pass_phrase_dialog -%>
SSLPassPhraseDialog <%= @ssl_pass_phrase_dialog %>
<% else -%>
SSLPassPhraseDialog builtin
Copy link
Contributor

Choose a reason for hiding this comment

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

hrm, yeah, I think you can collapse the whole thing by making 'builtin' the default.

@underscorgan
Copy link
Contributor

@dteirney @igalic yeah, I think having ssl_pass_phrase_dialog default to builtin will clean up the template (and is a reasonable default)

@dteirney if you could also update the unit tests at https://github.com/puppetlabs/puppetlabs-apache/blob/master/spec/classes/mod/ssl_spec.rb to exercise the new parameter that would be great!

@dteirney
Copy link
Author

I've made the change for builtin to be the default. Will add tests in the coming days. I presume we'll probably need to squash once everything is in place.

@underscorgan
Copy link
Contributor

@dteirney any update on the tests?

Yes, once everything is in place we'll want to squash down to a single commit

@igalic
Copy link
Contributor

igalic commented Dec 22, 2014

ping @dteirney

@dteirney
Copy link
Author

I'll look into the tests in the next few days, perhaps early next week. Been away on holiday for a while. Only just got back.

Defaults to 'builtin' to match the current behavior.
@dteirney
Copy link
Author

I've squashed down the commits and added some tests following the same pattern as in worker_spec.rb.

Is there anything else that anyone thinks needs to be done for this PR before it is committed?

As an aside, ssl_conf.erb still has the block around it. From another PR I was asked to take the IfModule block out. Including IfModule still seems to be the pattern for most modules though. Is anyone looking to do a PR to remove the IfModule blocks if that is no longer the preferred pattern?

underscorgan pushed a commit that referenced this pull request Dec 30, 2014
@underscorgan underscorgan merged commit 070595f into puppetlabs:master Dec 30, 2014
@underscorgan
Copy link
Contributor

Great, thanks @dteirney.

Regarding IfModules, it's one of the things we're planning on removing in 2.0.0: https://github.com/puppetlabs/puppetlabs-apache/blob/2.0.x/DESIGN.md#get-rid-of-all-ifmodule-directives

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.

4 participants