Skip to content
This repository has been archived by the owner on Jan 10, 2023. It is now read-only.

Added manage_service option #27

Merged
merged 1 commit into from
Feb 8, 2017

Conversation

mvisonneau
Copy link
Contributor

No description provided.

@alexjfisher
Copy link
Member

@mvisonneau Hi! Could you explain your motivation?

I'm not a fan of using if !defined.
Would a manage_service boolean parameter meet your needs?

@mvisonneau
Copy link
Contributor Author

@alexjfisher, sure well my issue here is while I use it with the puppetlabs/puppetdb module onto the same host. This is what we have on the other module :

https://github.com/puppetlabs/puppetlabs-puppetdb/blob/master/manifests/master/config.pp#L169-L173

if ! defined(Service[$puppet_service_name]) {
  service { $puppet_service_name:
    ensure => running,
  }
}

It causes a duplicate resources error if the puppetdb module is called first. In the meantime I would like to keep the ensure service running definition as part of the puppetserver as it makes more sense to me but sure, if you would rather like to have a manage_service option I'm fine with it, thanks :)

@bastelfreak
Copy link
Member

Hi,
I talked to @alexjfisher on IRC about that. In our opinion there shouldn't be an if ! defined() around the service resource. In general I'm fine with this pattern but in this case the module is dedicated to manage puppetserver and unpredictable things can happen if another module takes over the responsibility to manage the service.


Is it possible for you to change/enforce the order your classes are parsed? Handling puppetserver before puppetdb should solve the issue?

@bastelfreak
Copy link
Member

However I like the idea to manage the name of the service and I would merge that if you remove the if ! defined() and rename the commit.

@juniorsysadmin juniorsysadmin added needs-work not ready to merge just yet tests-fail labels Jun 12, 2016
@juniorsysadmin
Copy link
Member

@mvisonneau bump

@mvisonneau mvisonneau force-pushed the defined_service_check branch 2 times, most recently from 61bbb29 to c7e965b Compare January 9, 2017 09:02
@mvisonneau mvisonneau changed the title Added check onto service definition Added manage_service option Jan 9, 2017
@mvisonneau
Copy link
Contributor Author

let me know if this suits you well

@juniorsysadmin juniorsysadmin added needs-tests and removed needs-work not ready to merge just yet tests-fail labels Jan 9, 2017
@juniorsysadmin
Copy link
Member

Tests are failing on master not due to this change. It would be great if there was a unit test for this PR, but if you're not able to do this, please create a tracking Issue for it.

Copy link
Contributor

@vinzent vinzent left a comment

Choose a reason for hiding this comment

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

the manage_service param is a common pattern. looks good to me.

A rebase is needed and maybe fixing spec tests.

And adding a test for the param would also be super.

@mvisonneau
Copy link
Contributor Author

  • Rebase done
  • Spec tests created

@mvisonneau mvisonneau force-pushed the defined_service_check branch from 925a75c to ef512eb Compare February 8, 2017 11:45
Copy link
Contributor

@vinzent vinzent left a comment

Choose a reason for hiding this comment

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

looks good to me now

@vinzent vinzent merged commit c5d0400 into voxpupuli:master Feb 8, 2017
@vinzent
Copy link
Contributor

vinzent commented Feb 8, 2017

thank you @mvisonneau ! great to see rspec tests added.

@mvisonneau mvisonneau deleted the defined_service_check branch February 8, 2017 15:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants