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

Bump Apicurio Registry version to 2.4.2.Final #16

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

andreaTP
Copy link

First of all, thanks for this plugin!

Fix: Apicurio/apicurio-registry#3301

The significant changes are because ArtifactType is now mapped to a simple String to allow for user-defined extensions (e.g. we got requests for BigQuery).

@dmuharemagic
Copy link
Contributor

Thank you for the contribution to the project. Means a lot!
Left a comment to look out for. I'll be happy to integrate the additional changes if necessary.

@@ -88,18 +86,17 @@ class SchemaRegistryDownloadTaskSpecification extends AbstractFunctionalSpecific

then:
result.task(":$SchemaRegistryDownloadTask.TASK_NAME").outcome == TaskOutcome.SUCCESS
assertArtifactExistsAndIsReadable(metadata.outputPath, "${metadata.name}.${artifactType.extension}")
assertArtifactExistsAndIsReadable(metadata.outputPath, "${metadata.name}.${artifactType}")
Copy link
Contributor

@dmuharemagic dmuharemagic May 2, 2023

Choose a reason for hiding this comment

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

I agree with notion of making the artifact type more flexible, however we still need to deal with the file extensions properly.
When executing the download task , this would produce an output file name of e.g. Person.AVRO, which does not have the proper extension that corresponds to that format.
Perhaps we could be explicit about the extension by integrating it into the outputPath property of DownloadArtifact, and omit the outputFileName property?

Copy link
Author

Choose a reason for hiding this comment

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

What you are proposing seems reasonable to me.
Since I'm not familiar with the project, would you mind taking over this work?
Feel free to close this PR and come up with an alternative one, PR to my fork or whatever fit the best for you 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, I'll PR to your fork to keep this one open. Thanks once again 👍

Choose a reason for hiding this comment

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

hey @dmuharemagic is there any update on this change? Currently apicurio is still in a PoC phase at our client, but we could really use this fix so that we can use this gradle plugin as well. Otherwise we would have to create something similar ourselves

Copy link

@FinKingma FinKingma Jun 13, 2023

Choose a reason for hiding this comment

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

it also looks like the initial issue I create at apicurio registry side has been closed now. Would it help for traceability to create a new one here?
Apicurio/apicurio-registry#3301 (comment)

Copy link
Contributor

@dmuharemagic dmuharemagic Jun 25, 2023

Choose a reason for hiding this comment

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

@FinKingma It appears that the API changes resulting from the pull request at Apicurio/apicurio-registry#2963 have a more significant impact than originally anticipated. The ArtifactType is now directly mapped to a simple String in the internal API, and unfortunately, the current version of the Java SDK does not provide a way to retrieve the list of registered artifact types (although it is possible through the REST API, I will need to integrate the call using a custom HTTP client). As a result, the task will require more time to complete. Upgrading the Apicurio version cannot be separated from these implicit API changes. I have two options: either submit a pull request to the Apicurio repository to include the necessary call in their Java SDK or refactor the underlying communication with the registry using a custom HTTP client instead of the Java SDK. Either way, it will prolong the release of a new version, and I apologize for the delay. I am fully committed to resolving this issue and will provide the required upgrade soon.

FYI: I will close this PR and open a new issue/PR which will serve as the new integration point, considering this is a larger change.

Choose a reason for hiding this comment

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

@dmuharemagic ah bummer, but thanks for the headsup, could you link the new issue once it is created?

Copy link
Author

Choose a reason for hiding this comment

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

submit a pull request to the Apicurio repository to include the necessary call in their Java SDK

Sorry for the late feedback! If you can open an issue in apicurio-registry for the endpoint that is needed I'll be happy to take it!

Copy link
Author

Choose a reason for hiding this comment

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

@dmuharemagic Really sorry for the late feedback but this thread went into Spam 😥
I added the missing API to the Registry Client library, please let me know if there is anything left I can do to facilitate.

Copy link
Contributor

Choose a reason for hiding this comment

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

@andreaTP Sorry, missed your reply last night. Thank you for opening the issue and the quick implementation, really simplifies things on my end. 🙂
@FinKingma Sure, I'll link the new issue.

Copy link
Contributor

@dmuharemagic dmuharemagic left a comment

Choose a reason for hiding this comment

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

Additionally, we need to bump the plugin version located here to support a proper release. (https://github.com/croz-ltd/apicurio-registry-gradle-plugin/blob/main/plugin/gradle.properties).

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.

createdOn deserialize error with datestrings ending with 'Z' (UTC)
3 participants