Generate events using jsonschema2pojo and mustache template #58

rjalander · 2023-06-26T15:32:49Z

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

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

Note:

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)
Removed current Events(dev.cdevents.events) and Models(dev.cdevents.models) that are manually created earlier.

lapentad

lgtm
Just some comments on the CDEventsGenerator class which hold most of the logic here. But I don't see anything disruptive, apart from indentation/whitespaces/formatting (So anything you can see from the linting log)
I'll take a second look another day

lapentad · 2023-06-27T07:08:02Z

src/main/java/dev/cdevents/CDEventsGenerator.java

+        } catch (IOException e) {
+            throw new CDEventsException("Exception occurred while generating class file from Json schema ", e);
+        }
+        System.out.println("Rendered event-template has been written to file - "+classFile.getAbsolutePath());


Should this be logged with a logger or printing is fine?

yes, will be changing to logger for all the required logs

src/main/java/dev/cdevents/CDEventsGenerator.java

lapentad · 2023-06-27T07:22:26Z

src/main/java/dev/cdevents/CDEventsGenerator.java

+            String predicate = type[3];
+            String capitalizedSubject = StringUtils.capitalize(subject);
+            if(subject.equals("pipelinerun")){
+                capitalizedSubject = subject.substring(0, 1).toUpperCase() + subject.substring(1, 8) + subject.substring(8, 9).toUpperCase() + subject.substring(9);


Are you trying to capitalize only char 0 and char 8 in the string?

Suggested change

capitalizedSubject = subject.substring(0, 1).toUpperCase() + subject.substring(1, 8) + subject.substring(8, 9).toUpperCase() + subject.substring(9);

capitalizedSubject = StringUtils.capitalize(subject.substring(0, 8)) +

StringUtils.capitalize(subject.substring(8));

If you import a newer version of StringUtils you can capitalize the first character of a substring this way.

yes that's right using the StringUtils.capitalize() in the previous line, I can update to this simplified one based on your suggestion,
capitalizedSubject = capitalizedSubject.substring(0, 8) + StringUtils.capitalize(subject.substring(8));

src/main/java/dev/cdevents/CDEventsGenerator.java

afrittoli · 2023-06-27T08:16:33Z

@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 · 2023-06-27T16:43:41Z

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

rjalander · 2023-06-29T11:12:09Z

@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 · 2023-06-29T15:15:37Z

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 · 2023-07-05T06:53:31Z

pom.xml

@@ -105,6 +105,18 @@
 			<artifactId>jackson-databind</artifactId>
 		</dependency>

+		<dependency>
+			<groupId>commons-lang</groupId>
+			<artifactId>commons-lang</artifactId>


Please use latest versions as much as possible. In particular commons-lang3 supersedes commons-lang

https://mvnrepository.com/artifact/org.apache.commons/commons-lang3

aalmiray · 2023-07-05T06:55:45Z

pom.xml

@@ -128,6 +140,12 @@
 			<version>${packageurl.version}</version>
 		</dependency>

+		<dependency>
+			<groupId>com.github.spullara.mustache.java</groupId>


Is this dependency required at runtime or only during code generation? If the latter then change to <scope>provided</scope> as well as adding <optional>true</optional>, as this dependency should not leak to consumers.

aalmiray · 2023-07-05T06:57:19Z

pom.xml

+				</configuration>
+			</plugin>
+			<plugin>
+				<groupId>org.codehaus.mojo</groupId>


Based on the previous comment on the mustache dependency. Might be worth separating code generation from generated code consumption. That is, migrate this project to a multi module Maven setup, putting code generators in one module, and cdevents Java sdk in another. Otherwise we risk exposing the code generator and its dependencies as part of the runtime dependencies required by consumers of cdevents-java-sdk.

Updated the code as suggested with multi modules generator and sdk based on your commit

aalmiray

LGTM. However, the release workflow in .github/workflows/release.yml and the root pom.xml require some changes:

release.yml

change ./mvnw -ntp -B --file pom.xml -Prelease to ./mvnw -ntp -B --file pom.xml -Prelease -pl :cdevents-sdk-java-parent

pom.xml

update version.jreleaser.plugin to 1.7.0
update <pluginManagement> by adding

                <plugin>
                    <groupId>org.jreleaser</groupId>
                    <artifactId>jreleaser-maven-plugin</artifactId>
                    <version>${version.jreleaser.plugin}</version>
                </plugin>

aalmiray · 2023-07-13T08:53:32Z

Latest changes are 👍

rjalander · 2023-07-13T08:55:54Z

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

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

Co-authored-by: Lapenta Francesco Davide <37077655+lapentad@users.noreply.github.com> Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

aalmiray · 2023-07-14T12:54:53Z

generator/pom.xml

+            <scope>runtime</scope>
+        </dependency>
+
+        <!-- Test dependencies -->


The generator Maven module needs no test dependencies. These can be removed.

Actually I will create a Unit testing for generator code in the coming PR. Is that fine to keep this dependency.

It's great that unit tests will be added, maybe you can add the dependency along with the tests?

Indeed. It's a good practice to add only the minimum reqs for a given unit of work.

Done, Removed

aalmiray · 2023-07-14T12:57:55Z

sdk/pom.xml

+                </executions>
+            </plugin>
+            <plugin>
+                <groupId>org.apache.maven.plugins</groupId>


I think the library must remain Java 8 compatible. The parent pom defines compiler properties for Java 1.8. Remove this plugin definition.

Removed this plugin

Hold on a second. The moditect plugin was added because the codebase was set to Java 8 as a baseline. If the baseline moves to 11 then this plugin may be removed BUT and explicit module descriptor must be added.

@afrittoli has the Java 8 baseline changed recently?

The plugin I requested for removal is the compiler plugin because it explicitly overrides source/target compatibility to 11.

I'm not aware of any change from Java 8 to Java 11.

I remember we mentioned in the DEVELOPMENT readme "Java 9 is a minimum requirement to build the project",

@aalmiray is that readme need to be updated / any changes needed in the pom.xml ?

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

pom.xml

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

rjalander · 2023-07-21T08:44:27Z

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

rjalander · 2023-07-24T11:21:06Z

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?

afrittoli · 2023-07-24T13:25:15Z

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 · 2023-07-24T13:30:56Z

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 · 2023-07-28T18:11:30Z

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? 🙏

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

rjalander · 2023-08-22T08:24:08Z

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

afrittoli

Thanks @rjalander - it's great to see this happening!

I'm a bit worried about all the events being deleted and only a few ones being added as generated. All the tests are deleted too, and new one are added for the new generated code, which makes it harder to verify that the new code still behaves the same as the old one.

If you're confident that the new code preserves functionality, it's ok for me, hopefully this is a one-off for the transition to generated code.

rjalander · 2023-09-28T13:34:39Z

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

ok - let's go for it!

afrittoli · 2023-09-28T13:54:59Z

@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 · 2023-09-28T15:58:28Z

Yes we can Squash and merge.

rjalander · 2023-10-02T08:34:49Z

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

cdevents-bot · 2024-01-11T15:26:04Z

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

lapentad reviewed Jun 27, 2023

View reviewed changes

aalmiray requested changes Jul 5, 2023

View reviewed changes

rjalander requested a review from aalmiray July 12, 2023 17:05

aalmiray requested changes Jul 12, 2023

View reviewed changes

Jalander Ramagiri and others added 20 commits July 13, 2023 11:16

Changes to generate class file using template

9865d62

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

update CDEventsGenerator file

886070a

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

update CDEventsGenerator file

3653b32

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

Update phase for CDEventsGenerator main class

7366ba2

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

adding missing file to repo

fcb0514

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

Update src/main/java/dev/cdevents/CDEventsGenerator.java

8dc440a

Co-authored-by: Lapenta Francesco Davide <37077655+lapentad@users.noreply.github.com> Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

Update src/main/java/dev/cdevents/CDEventsGenerator.java

8bf6783

Co-authored-by: Lapenta Francesco Davide <37077655+lapentad@users.noreply.github.com> Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

update with review comments and some linter issues

8715628

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

fix linter issues

8e837e6

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

updating linter.yml

fb70e74

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

fix linter issues with MagicNumber

a594c13

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

fix linter issues with MagicNumber

04151e7

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

lint exclude list in array path

4e1c5ed

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

update FILTER_REGEX_EXCLUDE pattern

68f666b

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

update MaginNumners with constants

30c6654

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

ignoreFieldDeclaration for MagicNumber check-style

05e0dd3

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

cleanup existing/manual events to keep only gererated events

d78dfeb

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

Refacoring the code to use maven submodules for generator and sdk

a2e9beb

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

fix minor lint issue

81a7864

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

updating release files as per comment

ea383aa

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

rjalander requested review from aalmiray and lapentad July 14, 2023 10:29

aalmiray requested changes Jul 14, 2023

View reviewed changes

removing plugin definition from sdk/pom

9c10633

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

rjalander force-pushed the gen_sdk_v0_1_2 branch from 36a7509 to 9c10633 Compare July 14, 2023 14:02

afrittoli reviewed Jul 14, 2023

View reviewed changes

pom.xml Show resolved Hide resolved

removing test dependencies

8b4ec23

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

Merge branch 'main' into gen_sdk_v0_1_2

6571579

Jalander Ramagiri added 2 commits August 8, 2023 16:58

Update parent pom artifactId

4080307

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

Update sdk and generator pom artifact version

7aa2e7e

Signed-off-by: Jalander Ramagiri <jalander.ramagiri@est.tech>

rjalander requested review from aalmiray and afrittoli August 10, 2023 08:41

rjalander mentioned this pull request Aug 30, 2023

PART 2: Generate events using jsonschema2pojo and mustache template Nordix/sdk-java#1

Closed

rjalander mentioned this pull request Sep 27, 2023

Update Java SDK to CDEvents spec v0.2 #27

Closed

afrittoli reviewed Sep 27, 2023

View reviewed changes

afrittoli approved these changes Sep 28, 2023

View reviewed changes

afrittoli merged commit 739fb09 into cdevents:main Oct 2, 2023
3 checks passed

rjalander mentioned this pull request Oct 3, 2023

PART 2: Generate events using jsonschema2pojo and mustache template #60

Merged

cdevents-bot added the released Issue has been released label Jan 11, 2024

	capitalizedSubject = subject.substring(0, 1).toUpperCase() + subject.substring(1, 8) + subject.substring(8, 9).toUpperCase() + subject.substring(9);
	capitalizedSubject = StringUtils.capitalize(subject.substring(0, 8)) +
	StringUtils.capitalize(subject.substring(8));

Generate events using jsonschema2pojo and mustache template #58

Generate events using jsonschema2pojo and mustache template #58

Conversation

rjalander commented Jun 26, 2023 • edited Loading

lapentad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afrittoli commented Jun 27, 2023

rjalander commented Jun 27, 2023 • edited Loading

rjalander commented Jun 29, 2023

afrittoli commented Jun 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aalmiray left a comment

Choose a reason for hiding this comment

aalmiray commented Jul 13, 2023

rjalander commented Jul 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjalander commented Jul 21, 2023

rjalander commented Jul 24, 2023

afrittoli commented Jul 24, 2023

rjalander commented Jul 24, 2023

aalmiray commented Jul 28, 2023

rjalander commented Aug 22, 2023

afrittoli left a comment

Choose a reason for hiding this comment

rjalander commented Sep 28, 2023

afrittoli left a comment

Choose a reason for hiding this comment

afrittoli commented Sep 28, 2023

rjalander commented Sep 28, 2023

rjalander commented Oct 2, 2023

cdevents-bot commented Jan 11, 2024

rjalander commented Jun 26, 2023 •

edited

Loading

rjalander commented Jun 27, 2023 •

edited

Loading