-
Notifications
You must be signed in to change notification settings - Fork 14
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
The JWS installation option should allow you to exclude natives #97
Conversation
@guidograzioli Can you review and approve? |
I think this PR contains many same commits as #89? Can you check? |
@guidograzioli yes, it's designed to be merge after PR89. |
LGTM! |
@rpelisse I cloned the PR branch and tested it a bit, and found that there's one place you didn't add a I did notice another thing that we may want to address. If you run the script with the default |
@csutherl you are a god send! Thank you for spotting all of that. I think I've fixed everything and this PR should be good to go. However, if you can review (and /or test it) one last time, that certainly can't hurt :) |
@rpelisse I have a particular set of skills (spotting typos) 😆 My day is kind of full today, but I should be able to give it one more go this afternoon and provide some feedback. Thanks for being so quick to respond! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tested the latest commit and it still doesn't work unless you add the tomcat_native
check to roles/jws/tasks/install/zipfiles.yml
so that the zipfiles
list doesn't require the libtcnative.so. Without that check in place, you see the following failure when you run the playbook (having jws_version
and tomcat_install_method
defined):
TASK [middleware_automation.jws.jws : Install Jboss Web Server and required binaries from local zipfiles (install method: rhn_zipfiles] **************************************************************************************************************************************************************************************
changed: [localhost] => (item={'src': 'jws-5.6.0-application-server.zip', 'creates': '/opt/jws-5.6/tomcat/bin'})
failed: [localhost] (item={'src': 'jws-5.6.0-application-server-RHEL35-x86_64.zip', 'creates': '/opt/jws-5.6/tomcat/lib/libtcnative-1.so'}) => {"ansible_loop_var": "item", "changed": false, "item": {"creates": "/opt/jws-5.6/tomcat/lib/libtcnative-1.so", "src": "jws-5.6.0-application-server-RHEL35-x86_64.zip"}, "msg": "Source '/opt/jws-5.6.0-application-server-RHEL35-x86_64.zip' does not exist"}
register: local_path | ||
delegate_to: localhost | ||
|
||
- name: "Check downloaded archive local_path.stat.path }}/{{ zipfile }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The opening braces are still missing :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Darn! This was the very first thing I fixed up! It must have been squashed when I rebase :(
@csutherl OK, confirmed... Rebasing has removed all my changes... I guess "Don't rebase without testing". I'm going to update the PR. Sorry for that :( |
Reduce code duplication and ensure that native bits can be optional
@csutherl OK, added back the change, tested locally. But if you have the time to review again, that'll be awesome. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I appreciate you adding the restart notification, that works well too.
@csutherl Cool! No problem, it was a good idea to add notify :) Then I'll merge this right away :) |
Fixed #73 (and reduce code duplication)