voxpupuli / puppet-confluence

A puppet module to install confluence
https://forge.puppet.com/puppet/confluence
MIT License
21 stars 76 forks source link

centos 7 service template fixes #51

Closed hypertext418 closed 8 years ago

hypertext418 commented 8 years ago
bastelfreak commented 8 years ago

We do not provide a default for confluence::javahome in our init.pp, this gets dirty if the variable is empty. we should do something here to prevent this.

Could you squash the commits down to one or two (one for the user, one for the home)?

hypertext418 commented 8 years ago

I'm happy to squash the commits. I can get to that sometime later tonight or tomorrow. As for the javahome issue, both the JIRA and Stash modules handle it by making javahome a required parameter. Whether or not that's the best solution is up for debate. That said, in my opinion, setting JAVA_HOME in this service template is definitely best practice.

bastelfreak commented 8 years ago

I'm fine with that, can you make another PR that sets it as a required param (we've to merge that one before this one)?

@dhoppe do we need a major version bump if we change the param in that way? (IMO yes)

dhoppe commented 8 years ago

@bastelfreak I do not think so. The user is still set to 'confluence' (but manageable) and the java_home needs to be set in all of the Atlassian tools, anyway.

bastelfreak commented 8 years ago

I just checked the source code, it will currently fail if the var isn't set, so no major bump needed.

jyaworski commented 8 years ago

LGTM once squashed.

juniorsysadmin commented 8 years ago

@dmm92 Not to be a nagger, but I would like this in before cutting the next release to the Forge

hypertext418 commented 8 years ago

@juniorsysadmin not nagging at all. I said I'd do this a week ago. Sorry for the delay