-
Notifications
You must be signed in to change notification settings - Fork 3
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
Adding IBMMQ Discovery Agent #148
base: main
Are you sure you want to change the base?
Conversation
- name: username | ||
value: ush | ||
- name: password | ||
value: E5J8yGHjPTErikzOVZ0FiloP47FooNKQSTcTx5jQE-oM |
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.
Can these examples use ENV vars like ${SOME_USERNAME}
since that's the preferred way?
service/ibmmq-plugin/pom.xml
Outdated
<parent> | ||
<groupId>com.solace.maas</groupId> | ||
<artifactId>maas-event-management-agent-parent</artifactId> | ||
<version>1.3.2-SNAPSHOT</version> |
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.
please update this with latest version before merging, we are currently at version 1.4.2 and will soon release 1.5.0
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 other plugins don't appear to inherit from the EMA parent pom. I'll change this one to match those.
service/ibmmq-plugin/pom.xml
Outdated
</parent> | ||
|
||
<artifactId>ibmmq-plugin</artifactId> | ||
<version>0.0.2-SNAPSHOT</version> |
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.
@gregmeldrum question for you, shouldn't all plugin versions match maas-event-management-agent-parent
based on current architecture of EMA?
service/ibmmq-plugin/pom.xml
Outdated
<dependency> | ||
<groupId>com.solace.maas</groupId> | ||
<artifactId>plugin</artifactId> | ||
<version>1.3.2-SNAPSHOT</version> |
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.
this version needs to change before merge as well.
@@ -0,0 +1,21 @@ | |||
/** | |||
* | |||
*/ |
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 don't think these empty comment blocks are needed.
@Override | ||
public IbmMqHttpClient getClient(ConnectionDetailsEvent connectionDetailsEvent) { | ||
|
||
log.trace("Creating IBMMQ-HTTP client for messaging service [{}].", |
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.
please user event broker
instead of messaging service
public IbmMqQueueProcessor(MessagingServiceDelegateService messagingServiceDelegateService) { | ||
super(); | ||
this.messagingServiceDelegateService = messagingServiceDelegateService; | ||
log.debug("### In Queue Processor ###"); |
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.
This log seems to be a remnant of a debugging effort
@@ -22,6 +22,7 @@ | |||
<module>local-storage-plugin</module> | |||
<module>confluent-schema-registry-plugin</module> | |||
<module>application</module> | |||
<module>ibmmq-plugin</module> |
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.
Please also update the README files to mention this plugin and examples similar to the existing ones.
@moodiRealist |
There are some build problems with the current iteration. I tried fixing this and ran into some library version collisions. I think I fixed it all now. I can either update your original branch if you give me access to create a branch, or you can apply the following changes:
|
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.
Looks good to me. @gregmeldrum do you have any final comments?
Please make the following changes and then we'll merge it:
|
…EST client. Also, updated version to match parent.
I've updated some plugin internals & made the version current with the plugin. Please review @moodiRealist |
<!-- | ||
<spring.cloud.openfeign.version>4.0.6</spring.cloud.openfeign.version> | ||
<openfeign.jackson.version>13.0</openfeign.jackson.version> | ||
--> |
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.
If the commented out code is deadcode, please remove them.
service/ibmmq-plugin/pom.xml
Outdated
<maven.compiler.source>17</maven.compiler.source> | ||
<maven.compiler.target>17</maven.compiler.target> | ||
<solace.maas.ema.version>1.6.3-SNAPSHOT</solace.maas.ema.version> | ||
<spring.boot.version>3.1.8</spring.boot.version> |
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 believe we use spring.boot.version
3.1.9 in other places, was there a specific reason you didn't go with that version?
Catch up to upstream repo
Feature/merg with upstream
What is the purpose of this change?
How was this change implemented?
How was this change tested?
Is there anything the reviewers should focus on/be aware of?