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

7423 jms config #7443

Merged
merged 4 commits into from Dec 7, 2020
Merged

7423 jms config #7443

merged 4 commits into from Dec 7, 2020

Conversation

poikilotherm
Copy link
Contributor

What this PR does / why we need it:
This PR enables removing the configuration of the Java Messaging System Queue used for ingest from the installer and deploying it from inside the app instead. This greatly reduces the amount of bash glue necessary in containers.

Which issue(s) this PR closes:

Closes #7423

Special notes for your reviewer:
The release notes are missing. It's debateable wheter we want people to delete existing queue etc, as those are simply unused, doing no harm to anyone.

Suggestions on how to test this:
Just deploy and try ingest.

Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Nope.

Is there a release notes update needed for this change?:
Not yet.

Additional documentation:
None.

@poikilotherm poikilotherm added Type: Suggestion an idea Feature: Installer Feature: Installation Guide User Role: Sysadmin Installs, upgrades, and configures the system, connects via ssh labels Nov 30, 2020
@poikilotherm poikilotherm self-assigned this Nov 30, 2020
Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

If this works, great. I guess I only have two questions:

  1. This pull request is marked as "draft". Is it not ready for QA? My guess is that you're waiting for pull request 7418 datasourcedefinition #7422 to get merged.
  2. As a potential cleanup step (which I assume is optional) during upgrades, could some values be removed from domain.xml?

@poikilotherm poikilotherm moved this from Review 🦁 to Community Dev 💻❤️ in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) Nov 30, 2020
@poikilotherm
Copy link
Contributor Author

poikilotherm commented Nov 30, 2020

Hi @pdurbin,
sorry I left this in Code Review.

  1. This pull request is marked as "draft". Is it not ready for QA? My guess is that you're waiting for pull request 7418 datasourcedefinition #7422 to get merged.

Yes, it would be nice to have #7422 done first. And I need to do some more testing with this.

  1. As a potential cleanup step (which I assume is optional) during upgrades, could some values be removed from domain.xml?

We can do that. It should be only one asadmin call. These things should be added in a release note, which is one of the reasons this is in draft mode.

@coveralls
Copy link

coveralls commented Nov 30, 2020

Coverage Status

Coverage decreased (-0.0008%) to 19.399% when pulling 7139376 on poikilotherm:7423-jms-config into ee3b4f4 on IQSS:develop.

@poikilotherm poikilotherm marked this pull request as ready for review December 3, 2020 17:52
@poikilotherm
Copy link
Contributor Author

/jenkins test please 😄

@poikilotherm poikilotherm moved this from Community Dev 💻❤️ to Review 🦁 in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) Dec 3, 2020
@poikilotherm
Copy link
Contributor Author

poikilotherm commented Dec 3, 2020

@pdurbin looks like API tests which include ingest ran smooth on EC2. Do we want people to remove the config from their domain or just leave it there? It doesn't do anything considered harmful, just sitting around as a queue.

@pdurbin pdurbin self-assigned this Dec 4, 2020
@pdurbin
Copy link
Member

pdurbin commented Dec 7, 2020

@poikilotherm yes, the API tests are passing: https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-7443/7/testReport/

(There were some earlier failures that you, @donsizemore and I talked about but they seem to be unrelated to the code.)

I don't think we need to document how to remove the old config but if you feel differently, please go ahead and put it in a release note. Either way, let's at least have a release note that indicates that config has been moved into code. @kcondon asked to me add something like that last time, which resulted in 7e3498e if you'd like an example.

Finally, please merge the latest from develop. Thanks!

Copy link
Member

@pdurbin pdurbin 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. I didn't run the code myself but they API tests passed on Jenkins.

IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from Review 🦁 to QA 🔎✅ Dec 7, 2020
@kcondon kcondon self-assigned this Dec 7, 2020
@kcondon kcondon merged commit 4a5d6e0 into IQSS:develop Dec 7, 2020
IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from QA 🔎✅ to Done 🚀 Dec 7, 2020
@poikilotherm poikilotherm deleted the 7423-jms-config branch December 7, 2020 22:11
@djbrooke djbrooke added this to the 5.3 milestone Dec 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature: Installation Guide Feature: Installer Type: Suggestion an idea User Role: Sysadmin Installs, upgrades, and configures the system, connects via ssh
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Move Java Message Service configuration to application
5 participants