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

Add apicurio jsonserde support #37722

Merged
merged 2 commits into from
Dec 24, 2023

Conversation

carlesarnal
Copy link
Contributor

@carlesarnal carlesarnal commented Dec 13, 2023

This PR includes the initial support for json schema serialization using Apicurio Registry serdes. For now, autodiscovery of the channel and appropriate serde is not possible, so it must be manually configured.

@quarkus-bot quarkus-bot bot added area/core area/dependencies Pull requests that update a dependency file area/kafka labels Dec 13, 2023
@carlesarnal carlesarnal force-pushed the add-apicurio-jsonserde-support branch from 54da4ce to d9490e1 Compare December 13, 2023 19:03
@carlesarnal carlesarnal marked this pull request as ready for review December 15, 2023 10:16
@carlesarnal carlesarnal force-pushed the add-apicurio-jsonserde-support branch from 99d4de1 to 60992ca Compare December 15, 2023 10:18
@carlesarnal
Copy link
Contributor Author

@cescoffier do you mind approving the CI run? I have the quickstart ready locally and the tests passing for native as well.

@quarkus-bot quarkus-bot bot added the area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure label Dec 18, 2023

This comment has been minimized.

@carlesarnal carlesarnal force-pushed the add-apicurio-jsonserde-support branch from 0892fe3 to f412e69 Compare December 18, 2023 13:44
@quarkus-bot quarkus-bot bot added area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation labels Dec 18, 2023
@gsmet
Copy link
Member

gsmet commented Dec 18, 2023

@carlesarnal hey, thanks for this PR. Ideally, make sure CI passes in your fork before we approve the CI run here.

Also, probably a good idea to squash the commits to be a bit more semantic. Not sure if you need one big one or several ones but we don't need to see all the small iterations.

Thanks!

@carlesarnal
Copy link
Contributor Author

@carlesarnal hey, thanks for this PR. Ideally, make sure CI passes in your fork before we approve the CI run here.

Also, probably a good idea to squash the commits to be a bit more semantic. Not sure if you need one big one or several ones but we don't need to see all the small iterations.

Thanks!

Hey, yes, I realised now that the CI jobs are running on my fork, I'll make sure they pass there and squash the commits, tyvm.

@carlesarnal carlesarnal force-pushed the add-apicurio-jsonserde-support branch from cd67477 to 61c1919 Compare December 18, 2023 19:15
@quarkus-bot quarkus-bot bot added area/arc Issue related to ARC (dependency injection) area/docstyle issues related for manual docstyle review area/maven area/oidc area/rest area/vertx labels Dec 18, 2023
@carlesarnal carlesarnal force-pushed the add-apicurio-jsonserde-support branch from 61c1919 to f7f7268 Compare December 18, 2023 19:19
Copy link

github-actions bot commented Dec 18, 2023

🙈 The PR is closed and the preview is expired.

This comment has been minimized.

@carlesarnal carlesarnal force-pushed the add-apicurio-jsonserde-support branch from f7f7268 to aa7b2bd Compare December 19, 2023 08:10
@carlesarnal
Copy link
Contributor Author

@gsmet all the tests are green on my fork except for the JVM 17 on Windows that, for some reason cannot find a valid docker env. Any hint on what might be happening?

@carlesarnal carlesarnal force-pushed the add-apicurio-jsonserde-support branch from aa7b2bd to 12c13a3 Compare December 19, 2023 14:56
@carlesarnal
Copy link
Contributor Author

@gsmet all the tests are green on my fork except for the JVM 17 on Windows that, for some reason cannot find a valid docker env. Any hint on what might be happening?

Ok, I might have figured out the reason, I was defaulting the test execution to true no matter the profile, I have changed the default values appropriately, let's see what CI has to say, sorry about the noise.

@carlesarnal
Copy link
Contributor Author

@cescoffier at your convenience, CI on my fork is green, including native tests etc, so this is ready for your review. Thanks.

Copy link
Member

@cescoffier cescoffier left a comment

Choose a reason for hiding this comment

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

LGTM

It would be great to:

  1. implement serde discovery if possible (not totally sure it it possible)
  2. Make sure native is working as expected.

@carlesarnal
Copy link
Contributor Author

LGTM

It would be great to:

  1. implement serde discovery if possible (not totally sure it it possible)
  2. Make sure native is working as expected.
  1. It may be possible by making it depend on the schema location property since that property is only configurable for the json schema serializer. I would need some guidance on how to implement it.
  2. Native is working fine so far, the tests have been included in the native tests json and are passing on my fork and I already have the quickstart ready here and the native tests is passing there as well with no issues with a local build of Quarkus. The only requisite in this case is to add the json schema that will be used for serialization to the quarkus.native.resources.includes property, something like this.

This comment has been minimized.

Copy link

quarkus-bot bot commented Dec 21, 2023

Failing Jobs - Building bbee606

Status Name Step Failures Logs Raw logs Build scan
✔️ JVM Tests - JDK 17 🚧
JVM Tests - JDK 21 Build Failures Logs Raw logs 🚧

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 21 #

- Failing: extensions/scheduler/deployment 
! Skipped: extensions/quartz/deployment extensions/spring-scheduled/deployment integration-tests/main and 7 more

📦 extensions/scheduler/deployment

io.quarkus.scheduler.test.PausedSchedulerTest.testSchedulerPauseResume line 47 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: expected: <false> but was: <true>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)

@cescoffier cescoffier merged commit 4904645 into quarkusio:main Dec 24, 2023
52 of 53 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.7 - main milestone Dec 24, 2023
@carlesarnal carlesarnal deleted the add-apicurio-jsonserde-support branch December 26, 2023 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/arc Issue related to ARC (dependency injection) area/core area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/docstyle issues related for manual docstyle review area/documentation area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure area/kafka area/maven area/oidc area/reactive-messaging area/rest area/smallrye area/vertx
Projects
Development

Successfully merging this pull request may close these issues.

4 participants