Skip to content
This repository has been archived by the owner on Jun 28, 2018. It is now read-only.

refs #7779 - Updating to add support for pulp crane #1

Merged
merged 1 commit into from Oct 13, 2014

Conversation

bbuckingham
Copy link
Member

This is the initial PR to add the puppet-crane module.

This module does a simple config of the Pulp Crane application.

port => 5000,
priority => '03',
ssl => true,
ssl_cert => $crane::params::cert,
Copy link
Member

Choose a reason for hiding this comment

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

This means they're hardcoded. I'd at least set it to $crane::cert, but also strongly consider adding them as class parameters:

class crane::apache (
  $cert = $crane::cert,
  $key  = $crane::key,
  $ca   = $crane::ca,
) {

The benefit is that you can easily test this class without including the crane class and still pass in various parameters. That helps with performance. It can also help you when you use templates because they're already in scope and there's no need for the clunky scope.lookup function.

@ehelms
Copy link
Member

ehelms commented Oct 9, 2014

Ideally this would include a use_ssl param and setup a vhost for SSL only if that was specified. However, I'd be OK with sticking to SSL only for now but would encourage removing the direct dependency on Certs and just allow passing in the cert files. This will make this module simpler and more re-usable.

@ekohl
Copy link
Member

ekohl commented Oct 9, 2014

@ehelms that sounds like a very good idea and could easily be done by defaulting to undef instead of certs::*.

@bbuckingham bbuckingham force-pushed the issue-7779 branch 2 times, most recently from 54cb6e3 to 488c2a9 Compare October 10, 2014 22:06

6. Ensure commit message begins with 'Fixes #<redmine_issue_number>'

5. Push to your fork andaa submit a pull request.
Copy link
Member

Choose a reason for hiding this comment

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

s/andaa/and/


##Limitations

* EL6 (RHEL6 / CentOS 6)
Copy link
Member

Choose a reason for hiding this comment

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

Should probably add EL7 here as well.

@ekohl
Copy link
Member

ekohl commented Oct 13, 2014

An admin should enable travis integration so the tests are actually executed btw.


) inherits crane::params {

Class['certs'] ~>
Copy link
Member

Choose a reason for hiding this comment

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

I think you can get rid of this dependency now that you've reworked the certs and expecting them to be handed to Crane.

Copy link
Member

Choose a reason for hiding this comment

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

Also, one thing you could potential do is use the validators (https://github.com/puppetlabs/puppetlabs-stdlib -- search for validate) to ensure the certs are passed in

@bbuckingham bbuckingham force-pushed the issue-7779 branch 2 times, most recently from a9addef to 3b58306 Compare October 13, 2014 16:08
class crane (
$key = $crane::key,
$cert = $crane::cert,
$ca_cert = $crane::ca_cert,
Copy link
Member

Choose a reason for hiding this comment

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

Sorry I didn't catch this the first time! Conventionally these should be pointing at the params

@key = $crane::params::key,
@cert = $crane::params::cert,
@ca_cert = $crane::params::ca_cert,

@ehelms
Copy link
Member

ehelms commented Oct 13, 2014

APT

@ekohl
Copy link
Member

ekohl commented Oct 13, 2014

👍 looks good to me.

@bbuckingham
Copy link
Member Author

@ekohl, @ehelms, Thanks for the review & feedback!

bbuckingham added a commit that referenced this pull request Oct 13, 2014
refs #7779 - Updating to add support for pulp crane
@bbuckingham bbuckingham merged commit e440fbc into Katello:master Oct 13, 2014
@bbuckingham bbuckingham deleted the issue-7779 branch October 13, 2014 19:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants