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

The JWS installation option should allow you to exclude natives #97

Merged
merged 1 commit into from
Apr 27, 2022

Conversation

rpelisse
Copy link
Contributor

Fixed #73 (and reduce code duplication)

@rpelisse
Copy link
Contributor Author

@guidograzioli Can you review and approve?
@csutherl can you test to be sure that native is no longer an obligation?

@guidograzioli guidograzioli self-requested a review April 22, 2022 16:47
@guidograzioli
Copy link
Member

I think this PR contains many same commits as #89? Can you check?

@rpelisse
Copy link
Contributor Author

@guidograzioli yes, it's designed to be merge after PR89.

@guidograzioli
Copy link
Member

LGTM!

@rpelisse rpelisse added the bugfixes Fixes that resolve issues. SHOULD not be used for minor enhancements label Apr 25, 2022
@csutherl
Copy link
Collaborator

csutherl commented Apr 25, 2022

@rpelisse I cloned the PR branch and tested it a bit, and found that there's one place you didn't add a tomcat_native check which was causing the native zipfile to still be required. Adding a check for tomcat_native == True to the when block in roles/jws/tasks/install/zipfiles.yml#L16 makes it work as expected.

I did notice another thing that we may want to address. If you run the script with the default tomcat_native: False and then run it again after setting tomcat_native: True it unpacks the zip as expected, but doesn't notify a restart of the Tomcat service. I had to manually restart the process for tomcat_native to be picked up. If we think this behavior is ok, we should probably document that a restart is required (after someone checks to make sure it's not an env problem).

@rpelisse
Copy link
Contributor Author

@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 :)

@csutherl
Copy link
Collaborator

@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!

Copy link
Collaborator

@csutherl csutherl left a 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 }}"
Copy link
Collaborator

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 :(

Copy link
Contributor Author

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 :(

@rpelisse
Copy link
Contributor Author

@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
@rpelisse
Copy link
Contributor Author

@csutherl OK, added back the change, tested locally. But if you have the time to review again, that'll be awesome.

@csutherl csutherl self-requested a review April 27, 2022 14:39
Copy link
Collaborator

@csutherl csutherl left a 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.

@rpelisse
Copy link
Contributor Author

@csutherl Cool! No problem, it was a good idea to add notify :) Then I'll merge this right away :)

@rpelisse rpelisse merged commit fe7554a into main Apr 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfixes Fixes that resolve issues. SHOULD not be used for minor enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The JWS installation option should allow you to exclude natives
3 participants