install and setup on centos #1
Conversation
tasks/service.yml
Outdated
| {{ karaf_install_symlink }}/bin/start && sleep 5 | ||
| args: | ||
| chdir: "{{ karaf_install_symlink }}/bin" | ||
| creates: "{{ karaf_install_symlink }}/bin/karaf-wrapper" | ||
| environment: |
There was a problem hiding this comment.
If you are going to change how this is defined here, it should probably change everywhere for consistency.
https://github.com/Islandora-Devops/ansible-role-karaf/blob/master/tasks/service.yml#L33-L36
| @@ -1,13 +1,21 @@ | |||
| --- | |||
|
|
|||
| - name: Copy setenv script | |||
There was a problem hiding this comment.
Is this needed? It runs fine for me in my testing without setting this on Cent 7.
Probably doesn't hurt to add, but just wondering why it was added.
There was a problem hiding this comment.
I wonder if we are masking some issues with the Blazegraph and Fedora roles by setting JAVA_HOME here then? Since it seems like Karaf starts fine without it...
There was a problem hiding this comment.
I built the tasks based on the instructions in the Karaf Quick Guide which included this script. I simply included it because they did.
There was a problem hiding this comment.
Okay cool. 👍🏻
Just wanted some detail why its there.
defaults/main.yml
Outdated
|
|
||
| karaf_java_packages: | ||
| karaf_java_packages_yum: |
There was a problem hiding this comment.
I'd like to see this remain karaf_java_pacakges and then set it to the appropriate value for cent or for ubuntu, same with karaf_java_home.
I'm a fan of that way this role handles OS specific settings, so they can be overridden in a playbook and they can get sensible default values.
Maybe something like this:
https://github.com/geerlingguy/ansible-role-mysql/blob/master/tasks/variables.yml
There was a problem hiding this comment.
Testing an alternative now.
There was a problem hiding this comment.
I should note that my recent Grok PR avoided this by simply listing the packages in repo-specific tasks. Should I go back and revise it to be more like the I just committed?
Now imports variable values for Debian and RedHat dynamically. Also cleans out legacy environment variables, set using the setenv script.
defaults/main.yml
Outdated
| @@ -13,14 +13,12 @@ karaf_log_path: ${karaf.data}/log/ | |||
|
|
|||
| karaf_logging_template: org.ops4j.pax.logging.cfg | |||
|
|
|||
| karaf_java_home: "{% if ansible_os_family == 'RedHat' %}/usr/lib/jvm/java-1.8.0-openjdk/{% else %}/usr/lib/jvm/java-8-openjdk-amd64{% endif %}" | |||
| karaf_java_home: /usr/lib/jvm/java-8-openjdk-amd64 | |||
There was a problem hiding this comment.
Should all these variables be removed as they are now set in the various vars files?
There was a problem hiding this comment.
You are right, since whatever value the have in defaults will get overridden by the vars value due to the include. (I think.)
vars/RedHat.yml
Outdated
| @@ -0,0 +1,7 @@ | |||
| --- | |||
|
|
|||
| karaf_java_home: /usr/lib/jvm/java-1.8.0-openjdk/ | |||
There was a problem hiding this comment.
Is the / here extraneous or should the same variable in the Debian.yml have one on its end? Might be nothing.
There was a problem hiding this comment.
It is probably extraneous. Both ubuntu and centos provisioned fine with the current settings. I can try removing it and testing centos again...
|
I like the new approach for variables. The only problem I have now is that the variables defined with Its a little ugly, but thats what this is avoiding: |
|
The conditional check isn't that ugly. I'll toss it in, but we will need to either delete or comment out the existing values in defaults. |
|
Maybe just comment them out, with a note that they are set to a distro specific variable... |
|
Maybe something like this: |
|
👍 looks awesome to me |
|
Travis is red, but I'm going to merge anyway since the testing branch isn't in yet. |
No description provided.