-
Notifications
You must be signed in to change notification settings - Fork 573
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
new kie-server-maven-plugin project : deploy, dispose and replace #1063
Conversation
Can one of the admins verify this PR? Comment with 'ok to test' to start the build. |
3 similar comments
Can one of the admins verify this PR? Comment with 'ok to test' to start the build. |
Can one of the admins verify this PR? Comment with 'ok to test' to start the build. |
Can one of the admins verify this PR? Comment with 'ok to test' to start the build. |
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 really good. Do you mind adding a Readme file describing it usage?
releaseId.setVersion(project.getVersion()); | ||
kieContainer.setReleaseId(releaseId); | ||
|
||
KieServerConfigItem configItem = new KieServerConfigItem(); |
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 am thinking that we should make this optional and add this config property when it is given otherwise it can easily replace one set in jar's deployment descriptor. wdyt?
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.
Totally agree. The runtime strategy is on kie-deployment-descriptor.xml too, and it's the natural default value.
import org.apache.maven.plugins.annotations.Mojo; | ||
|
||
/** | ||
* Replace the k-jar artifact in existing container |
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.
Update instead of Replace :)
ok to test |
try { | ||
client = KieServicesFactory.newKieServicesClient(config); | ||
} catch (RuntimeException kieEx) { | ||
throw new MojoFailureException("error on establish connection with remote server: " + kieEx.getMessage()); |
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.
Putting only a message from the original exception may lead to losing stacktrace - and the root cause. Could you rather incorporate the cause into the MojoFailureException, please?
<parent> | ||
<groupId>org.kie.server</groupId> | ||
<artifactId>kie-server-parent</artifactId> | ||
<version>7.2.0-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.
@fax4ever could you please update that to 8.0.0-SNAPSHOT as master was just moved to 8. Once it is merged we back port that to 7.2.x so it gets included in upcoming 7.2 community release
@fax4ever really nice Readme, thanks! |
Jenkins retest this |
Can one of the admins verify this PR? Comment with 'ok to test' to start the build. |
1 similar comment
Can one of the admins verify this PR? Comment with 'ok to test' to start the build. |
Jenkins retest this |
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.
great work @fax4ever
for unmanaged (direct process server) deploy.
fixing license header
kie-server context path is now configurable
deploy runtime strategy is now configurable
rename replace goal in update