-
Notifications
You must be signed in to change notification settings - Fork 24
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
ES-1992: migrate to corda runtime plugin #122
ES-1992: migrate to corda runtime plugin #122
Conversation
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.
PR title failed to match regex -> ^((CORDA|EG|ENT|INFRA|CORE|ES)-\d+)(.*)
build.gradle
Outdated
cordaRuntimePluginWorkspaceDir = "workspace" | ||
composeFilePath = "config/combined-worker-kafka-compose.yaml" | ||
// Alternatively, you can use a Database-only combined worker: | ||
// composeFilePath = "config/combined-worker-compose.yaml" |
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.
Do we need to provide an example of no-kafka deployment?
# Specify the version of the CSDE gradle plugin to use | ||
csdePluginVersion=1.2.0-beta-+ | ||
# Specify the version of the Corda runtime Gradle plugin to use | ||
cordaGradlePluginVersion=5.2.0.0-RC02 |
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 and other versions will need to be updated after 5.2 release
- kafka | ||
- kafka-create-topics | ||
environment: | ||
JAVA_TOOL_OPTIONS: -agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=*:5005 |
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 piece is missing in the examples provided in the plugin's readme.
This is necessary to enable debugger connection on port 5005 - to provide the string in the JAVA_TOOL_OPTIONS variable.
For some reason, if we specify it as one of the args in the command
list, it doesn't seem to be applied by JVM (as it does when started on a local JVM outside container), and debug connection fails.
@@ -1,25 +1,22 @@ | |||
# CSDE-cordapp-template-kotlin | |||
# release-V5 cordapp-template-kotlin |
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.
previously this was the name of the repository; now it is a specific branch name where the code would live, and the target repo name
@@ -1,6 +1,6 @@ | |||
package com.r3.developers.csdetemplate.utxoexample.contracts | |||
package com.r3.developers.cordapptemplate.utxoexample.contracts |
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.
rename package - change csdetemplate
to cordapptemplate
; I'm open for suggestions of a better name
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.
Works for me
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.
Ditto
build.gradle
Outdated
cordaCliBinDir = "${System.getProperty("user.home")}/.corda/cli" | ||
|
||
// Alternatively, you can use a Database-only combined worker: | ||
// composeFilePath = "config/combined-worker-compose.yaml" |
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.
We have an example of a Database-only compose file in the plugin's readme; the link to the readme was added to this repo's README.
The question is - do we still want to provide a no-kafka example here in the template repo, including this comment and the compose yaml file in the config?
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.
We should steer people to the Kafka enabled version, as it's quicker. We then offer the db only version to those who need it.
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 think it's fine to just have the Kafka version here. The non-Kafka version remains documented with the Gradle plugin.
@@ -110,7 +103,7 @@ allprojects { | |||
publishing { | |||
publications { | |||
maven(MavenPublication) { | |||
artifactId "corda-CSDE-kotlin-sample" | |||
artifactId "corda-dev-template-kotlin-sample" |
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'm not sure if this is being used anywhere; publishing is disabled in the pipeline
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 can't really see why it would ever be published.
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
build.gradle
Outdated
cordaCliBinDir = "${System.getProperty("user.home")}/.corda/cli" | ||
|
||
// Alternatively, you can use a Database-only combined worker: | ||
// composeFilePath = "config/combined-worker-compose.yaml" |
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.
We should steer people to the Kafka enabled version, as it's quicker. We then offer the db only version to those who need it.
@@ -1,6 +1,6 @@ | |||
package com.r3.developers.csdetemplate.utxoexample.contracts | |||
package com.r3.developers.cordapptemplate.utxoexample.contracts |
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.
Works for me
build.gradle
Outdated
cordaCliBinDir = "${System.getProperty("user.home")}/.corda/cli" | ||
|
||
// Alternatively, you can use a Database-only combined worker: | ||
// composeFilePath = "config/combined-worker-compose.yaml" |
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 think it's fine to just have the Kafka version here. The non-Kafka version remains documented with the Gradle plugin.
@@ -110,7 +103,7 @@ allprojects { | |||
publishing { | |||
publications { | |||
maven(MavenPublication) { | |||
artifactId "corda-CSDE-kotlin-sample" | |||
artifactId "corda-dev-template-kotlin-sample" |
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 can't really see why it would ever be published.
@@ -0,0 +1,32 @@ | |||
version: '2' |
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.
Let's just have the Kafka version...
@@ -0,0 +1,72 @@ | |||
version: '2' |
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've got some ideas for improvements we can make with this file but they can wait for another day.
@@ -1,6 +1,6 @@ | |||
package com.r3.developers.csdetemplate.utxoexample.contracts | |||
package com.r3.developers.cordapptemplate.utxoexample.contracts |
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.
Ditto
https://r3-cev.atlassian.net/browse/ES-1992
Not for merge!
The goal is to migrate this code to a different repository; PR raised only to allow reviews of the changes.