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

#854 - mvn plugin - allow schemas composition #855

Merged

Conversation

@kanly
Copy link
Contributor

commented Jul 27, 2018

As for #854

To add dependencies it's enough to use the same Schema.Parser() to parse dependencies and subjects.
his code wraps the creation of a new Schema.Parser and if dependencies are specified it creates a new parser with dependencies.
I decided to create a new parser for each subject both for simplicity and to don't allow to have dependencies across subjects

@ConfluentCLABot

This comment has been minimized.

Copy link

commented Jul 27, 2018

It looks like @kanly hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement here.

Once you've signed reply with [clabot:check] to prove it.

Appreciation of efforts,

clabot

@kanly

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2018

[clabot:check]

@ConfluentCLABot

This comment has been minimized.

Copy link

commented Jul 27, 2018

@confluentinc It looks like @kanly just signed our Contributor License Agreement. 👍

Always at your service,

clabot

@mageshn

This comment has been minimized.

Copy link
Member

commented Jul 27, 2018

@kanly Thanks a lot for the PR. A feature like this would be a great addition. A couple of questions

  1. Does this new property apply to all the goals? IMO, we should limit this to only the register goal.
  2. Do the imports apply to all the subject schemas? I think it's much more meaningful to be able to specify imports for each subject schema since we will register the whole schema in a subject.

@mageshn mageshn self-requested a review Jul 27, 2018

@kanly

This comment has been minimized.

Copy link
Contributor Author

commented Jul 28, 2018

@mageshn thanks for the prompt answer!

  1. I believe that this is also needed for the verify goal, I believe that it wouldn't work otherwise
  2. This is a good point, but I believe that we could add this option and also give the possibility to have common imports (for the sake of easy configuration).
    This second option would also be consistent with the avro plugin that uses a single Schema.Parser to parse all of them, the output of each parsing is only the passed schema with all the dependencies (assuming that they were previously parsed).

This is my point of view as an user, you're the expert here though

@cricket007

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2018

Question: Does this only apply to AVSC?

In AVDL you can already reference record types within a single protocol and do import statements on both avdl and avsc files

@kanly

This comment has been minimized.

Copy link
Contributor Author

commented Sep 7, 2018

@cricket007 sorry for the slow response. This is for schema registry maven plugin which as far as I understood works with avsc only.

@mageshn any news on this?

@mawek

This comment has been minimized.

Copy link

commented Oct 8, 2018

When could we expect this feature to be merged?

@mageshn

This comment has been minimized.

Copy link
Member

commented Oct 8, 2018

@kanly I would still prefer to have this localized to the goals that are required - register, verify. Also, it would be better if we could allow specifying the imports at the individual subject schema. I certainly agree that ability to specify common import might be useful as well.

@kanly

This comment has been minimized.

Copy link
Contributor Author

commented Nov 30, 2018

@mageshn sorry for the delay... busy times :)
Does this new version solves your concerns?

@cricket007

This comment has been minimized.

Copy link
Contributor

commented Dec 3, 2018

Following up here.

as far as I understood works with avsc only.

Using AVDL and the idl-protocol goal of the avro-maven-plugin, it will generate AVSC, which you can then register, as normal from this plugin.

@artjock

This comment has been minimized.

Copy link

commented Feb 11, 2019

Hi @mageshn, any updates on this?

@zhenik

This comment has been minimized.

Copy link

commented Mar 21, 2019

I think current implementation does support only 1 level of nested dependencies.
What if it is tree and dependencies has own dependencies and it could be shared among other schemas?

dependencies -> shared schemas

@kdehairy

This comment has been minimized.

Copy link

commented Jun 20, 2019

I think current implementation does support only 1 level of nested dependencies.
What if it is tree and dependencies has own dependencies and it could be shared among other schemas?

dependencies -> shared schemas

I don't believe this is correct. All it requires is ordered list of dependencies. If A references B, and B references C, then the order of the list should be: C, B, A. It won't work in other order.

AFAIK, avro plugin requires the same. So it is inline with the expectations of devs working with avro related stuff.

@kdehairy

This comment has been minimized.

Copy link

commented Jun 20, 2019

@mageshn Any updates on this pull request?

Paolo Mazzoncini
To add dependencies it's enough to use the same Schema.Parser() to pa…
…rse dependencies and subjects.

This code wraps the creation of a new Schema.Parser and if dependencies are specified it creates a new parser with dependencies.

I decided to create a new parser for each subject both for simplicity and to don't allow to have dependencies across subjects

@kanly kanly force-pushed the kanly:mvn-plugin-schemas-allow-composition branch from 43f89b2 to 85fa595 Jul 1, 2019

@kanly kanly changed the base branch from 4.1.x to 5.3.x Jul 1, 2019

@kanly

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2019

I've rebased the changes on 5.3.x as it seems the latest version.

We are currently using a forked version with this patch in our company, we would like to fix this.

Waiting for more feedback :)

@mageshn mageshn requested a review from rayokota Jul 30, 2019

@rayokota
Copy link
Member

left a comment

Thanks for the PR @kanly ! LGTM

@rayokota rayokota merged commit 226cf86 into confluentinc:5.3.x Aug 27, 2019

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.