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

transport protocol support #18

Merged
merged 11 commits into from
Sep 15, 2023
Merged

transport protocol support #18

merged 11 commits into from
Sep 15, 2023

Conversation

lumber1000
Copy link
Member

No description provided.

build.gradle Outdated Show resolved Hide resolved
build.gradle Show resolved Hide resolved
@@ -29,8 +29,9 @@ import java.nio.file.Path
@AutoService(IPipelineCodecFactory::class)
class XmlPipelineCodecFactory : IPipelineCodecFactory {
override val settingsClass: Class<out IPipelineCodecSettings> = XmlPipelineCodecSettings::class.java
@Deprecated("Please migrate to the protocols property")
override val protocol: String = PROTOCOL
Copy link
Member

Choose a reason for hiding this comment

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

Please, do the migration

build.gradle Outdated Show resolved Hide resolved
Copy link
Member

@OptimumCode OptimumCode left a comment

Choose a reason for hiding this comment

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

Please, correct test and update permissions for gradlew file (you can find the exact error in checks in PR)

build.gradle Show resolved Hide resolved
@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

Copy link
Member

@OptimumCode OptimumCode left a comment

Choose a reason for hiding this comment

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

Could you please try to migrate to sailfish-common from sailfish-core?

Also, please close all the resolved conversations

Copy link
Member

Choose a reason for hiding this comment

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

Please, add jvm target for kotlin tasks - there is a warning during compilation

 'compileTestJava' task (current target is 11) and 'kaptGenerateStubsTestKotlin' task (current target is 1.8) jvm target compatibility should be set to the same Java version.

build.gradle Outdated
@@ -164,11 +150,39 @@ compileTestKotlin {
}
}

clean {
clean {+6
Copy link
Member

Choose a reason for hiding this comment

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

Please, clean up code

val map = message.body
val xmlString = Xml.toXml(map)

validator.validate(xmlString.toByteArray())
Copy link
Member

Choose a reason for hiding this comment

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

I think we can (and should) reuse this buffer when creating RawMessage below. Also, we must use correct charset when converting string to bytes

eventId = message.eventId,
metadata = message.metadata,
protocol = XmlPipelineCodecFactory.PROTOCOL,
body = Unpooled.copiedBuffer(xmlString, xmlCharset)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to copy the result bytes? I think we can wrap the bytes that we used for validator.validate method call

Copy link
Member

@OptimumCode OptimumCode 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. Let's wait for th2-codec dev release and then release this component

build.gradle Outdated Show resolved Hide resolved
build.gradle Outdated
Comment on lines 116 to 118
testImplementation "org.junit.jupiter:junit-jupiter-api:$junitVersion"
testImplementation "org.junit.jupiter:junit-jupiter-params:$junitVersion"
testRuntimeOnly "org.junit.jupiter:junit-jupiter-engine:$junitVersion"
Copy link
Member

Choose a reason for hiding this comment

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

This all can be replaced with

Suggested change
testImplementation "org.junit.jupiter:junit-jupiter-api:$junitVersion"
testImplementation "org.junit.jupiter:junit-jupiter-params:$junitVersion"
testRuntimeOnly "org.junit.jupiter:junit-jupiter-engine:$junitVersion"
testImplementation "org.junit.jupiter:junit-jupiter:$junitVersion"

Comment on lines 116 to 118
testImplementation "org.junit.jupiter:junit-jupiter-api:$junitVersion"
testImplementation "org.junit.jupiter:junit-jupiter-params:$junitVersion"
testRuntimeOnly "org.junit.jupiter:junit-jupiter-engine:$junitVersion"
Copy link
Member

Choose a reason for hiding this comment

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

Why have you removed all these dependencies? kotlin-test-junit5 does provide junit-jupiter-api and junit-jupiter-engine but it does not provide junit-jupiter-params

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably junit-jupiter-params is not used in this project. Compilation succeed and all tests are passed. So we don't need this dependency.

@lumber1000 lumber1000 merged commit cd0406c into dev-version-2 Sep 15, 2023
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants