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 configurable service name #455

Closed
wants to merge 1 commit into from

Conversation

jstaph
Copy link

@jstaph jstaph commented Sep 22, 2014

This module works very well with the SCL packaged version of NGINX, but needs just one additional point of change. I've added a few lines to allow for adjustment of the service name.

@3flex
Copy link
Contributor

3flex commented Sep 22, 2014

Looks great! To simplify the change slightly could I ask that the new service_name parameter be added as an additional attribute on the service declaration, instead of as the name of the service? e.g.:

service {'nginx':
  name => $service_name,
  ...
}

Also I'm mindful of what's coming in #453. For this reason can you isolate this change to just a new parameter in service.pp as described? That will still let you configure the name and not conflict with that PR. So just the change to that file, and the spec test updates.

Thanks!

@jstaph
Copy link
Author

jstaph commented Sep 23, 2014

Ah, I should have had a look at the other PR's first. I think I've captured what you'd like, but I'm very happy to make further adjustments if I haven't. Thanks!

@3flex
Copy link
Contributor

3flex commented Sep 23, 2014

That's no problem, you can't be expected to review every other PR to make sure yours doesn't conflict!

I'm going to hold back a day or two until #453 is finalized to reduce churn there, I hope that's OK. In the meantime please squash your changes down a single commit for merging. Thanks!

@jstaph jstaph force-pushed the configurable_service_name branch from 17c844f to 588d55a Compare September 23, 2014 22:26
@jstaph
Copy link
Author

jstaph commented Sep 23, 2014

Squashed! It definitely makes sense to wait for #453, I'm in no rush. Thanks!

@3flex
Copy link
Contributor

3flex commented Dec 8, 2014

Hi @jstaph sorry I've let this slide, I had totally forgotten to come back and check this.

I just noticed that while you'd set a value in params.pp it's not referenced anywhere. So you can take that out. Also you should add a $service_name parameter to init.pp and pass that through to the nginx::service class (see around lines 109 & 289 of init.pp in master). You should also rebase before making these changes.

Soon as those changes are done I'll merge. Thanks for the code and sorry for the delay!

@3flex
Copy link
Contributor

3flex commented Dec 8, 2014

FYI I need to merge #516 before you'll see the relevant line (289) in init.pp I mentioned - once Travis passes that PR and I've merged you can rebase on master and you'll see the line I mentioned.

Cheers!

@3flex 3flex mentioned this pull request Dec 18, 2014
@3flex
Copy link
Contributor

3flex commented Dec 18, 2014

Created a new PR which addresses my comments: #534

@jstaph
Copy link
Author

jstaph commented Dec 19, 2014

@3flex,

Wow, I completely lost track of time. Many apologies!

Thanks for #534, and again, sorry I was away so long.

@3flex 3flex closed this in #534 Dec 20, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants