cdevents / sdk-java

Java SDK for CDEvents
Apache License 2.0
5 stars 6 forks source link

Generate events using jsonschema2pojo and mustache template #58

Closed rjalander closed 11 months ago

rjalander commented 1 year ago

Initial changes to generate events using jsonschema2pojo and mustache template, /src/main/resources/template/event-template.mustache

pom.xml has the plugin to generate jsonschema2pojo and plugin to run the main class from CDEventsGenerator to create events using mustache template.

Running ./mvnw verify

  1. Generates models using jsonschema2pojo for each event under - dev.cdevents.models.generated.subject.predicate
  2. Generates events from mustache template and placed under - dev.cdevents.events.generated

Note:

  1. Generating only 3 events for an initial review and schema files for the same are placed under - /src/main/resources/schema (This will essentially point to spec repo to generate all the events once reviewed)
  2. Removed current Events(dev.cdevents.events) and Models(dev.cdevents.models) that are manually created earlier.
afrittoli commented 1 year ago

@rjalander thank you, excited to see this happening!! I haven't checked the code, but one comment I have is that it would be nice to add a CI job that generates the code and checks if the code is up to date and fails if not.

rjalander commented 1 year ago

Thank you for your comment @afrittoli Currently it generates the code from the Schemas as part of "Test/Unit Test" CI job above, and I can add more Unit tests to validate the generated code.

Build Job image

rjalander commented 1 year ago

@afrittoli is this a good idea to keep a release branch of Java-sdk as of now before this PR gets merged into main as generated code for v0.1.2 version ?

afrittoli commented 1 year ago

I would recommend continuing regular development on main. We can push a tag for v0.1.2 and if in future we need more v0.1.x patches we can create a branch for that.

aalmiray commented 1 year ago

Latest changes are 👍

rjalander commented 1 year ago

Thank you all for your review and comments,

@aalmiray @afrittoli Can you please approve this PR If no other comments, I will be creating another PR which generates all other events pointing to v0.1.2 schemas of spec repository as submodule

rjalander commented 1 year ago

Thank you all for the review, @afrittoli @aalmiray Could you please approve If no other comments to address.

rjalander commented 1 year ago

Also the Spinnaker PR; https://github.com/spinnaker/echo/pull/1295, is dependent on the Java-SDK to produce CDevents from Spinnaker. I will create another PR to generate all other events once this PR is merged. Can we plan release of v0.1.2 to maven central to solve the Spinnaker dependency first?

afrittoli commented 1 year ago

Also the Spinnaker PR; spinnaker/echo#1295, is dependent on the Java-SDK to produce CDevents from Spinnaker. I will create another PR to generate all other events once this PR is merged. Can we plan release of v0.1.2 to maven central to solve the Spinnaker dependency first?

Hello @rjalander - yes, this does not seem to me like a dependency for v0.1.2 - I think we already agreed that the SDK is ready for v0.1.2. Is there anything else holding the release? Would you be happy to do the release or would you like me to do it?

rjalander commented 1 year ago

Ok, that will be great If we do release with the master changes as of now, yes can you please create a new release.

aalmiray commented 1 year ago

Not a blocker for releasing 0.1.2 but, given the contributions I've made to the project so far, could I be added as a contributor to the root pom.xml? 🙏

rjalander commented 1 year ago

@afrittoli @aalmiray please let me know If any other comments that needs to be addressed to get this PR approved/merged.

rjalander commented 11 months ago

In the next PR I will be adding other generated events which will have the same tests running to make sure the generated code works as expected. I am good with the changes as this will have test for each generated event to verify in the next PR.

afrittoli commented 11 months ago

@aalmiray If it's ok for you I would go ahead and merge this. @rjalander is it ok to squash this into one or do you want more commits? I would prefer not to keep all the 27 ones though

rjalander commented 11 months ago

Yes we can Squash and merge.

rjalander commented 11 months ago

@afrittoli can you please merge this PR, I will create a follow up PR for the next set of changes.

cdevents-bot commented 8 months ago

🎉 This issue has been resolved in v0.3.0 (Release Notes)