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

[java gen] - Support generated annotations #4406

Merged

Conversation

fabiobrz
Copy link
Contributor

@fabiobrz fabiobrz commented Sep 8, 2022

Description

The changes are related to those in #4348
The generator will now generate the expected annotations, i.e. those defined in io.fabric8.generator.annotation, the same ones that a reverse CRD generation from the Java model take into account.

Fix #4384

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change
  • Chore (non-breaking change which doesn't affect codebase;
    test, version modification, documentation, etc.)

Checklist

  • Code contributed by me aligns with current project license: Apache 2.0
  • I Added CHANGELOG entry regarding this change
  • I have implemented unit tests to cover my changes
  • I have added/updated the javadocs and other documentation accordingly
  • No new bugs, code smells, etc. in SonarCloud report
  • I tested my code in Kubernetes
  • I tested my code in OpenShift

I can suash the commits as soon as this is ready for merge or even earlier in case we agree on it.
FYI @andreaTP, CC @manusa

@fabiobrz fabiobrz force-pushed the java-gen.support-validation-annotations branch 3 times, most recently from 6058508 to 320cac5 Compare September 8, 2022 14:17
@fabiobrz fabiobrz marked this pull request as ready for review September 8, 2022 15:14
Copy link
Member

@andreaTP andreaTP left a comment

Choose a reason for hiding this comment

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

A couple of minor cleanups, but this is great work! Thanks a ton @fabiobrz !

@fabiobrz fabiobrz force-pushed the java-gen.support-validation-annotations branch 3 times, most recently from 2d188d8 to 150ddf6 Compare September 12, 2022 08:31
@fabiobrz
Copy link
Contributor Author

fabiobrz commented Sep 12, 2022

Hi @andreaTP and @manusa - latest changes pushed to address your review comments. Let me know if you spot anything else.

BTW - can I squash the commits now?

Copy link
Member

@andreaTP andreaTP left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this contribution @fabiobrz !
LGTM!

@fabiobrz
Copy link
Contributor Author

Thanks a lot for this contribution @fabiobrz ! LGTM!

Welcome @andreaTP - squashing my commits then!

@fabiobrz fabiobrz force-pushed the java-gen.support-validation-annotations branch from 150ddf6 to 7d9040f Compare September 12, 2022 10:48
@fabiobrz
Copy link
Contributor Author

fabiobrz commented Sep 12, 2022

Hi @manusa - one e2e tests failed after I squashed my commits, although it was green already before that.
I'll try to re-spin a run, but IMHO it should be an intermittent/random failure.

@fabiobrz fabiobrz closed this Sep 12, 2022
@fabiobrz fabiobrz reopened this Sep 12, 2022
@fabiobrz fabiobrz closed this Sep 12, 2022
@fabiobrz fabiobrz reopened this Sep 12, 2022
@andreaTP
Copy link
Member

cc. @manusa looking forward for your input here 🙏

Copy link
Member

@manusa manusa left a comment

Choose a reason for hiding this comment

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

LGTM, thx!

I think it would be worth adding a note on the the breaking changes list, since some users who might be using the Java generator, rely on the javax validation annotations that are no longer added.

This also raises the question: as a developer relying on the java generator, how can I further transform the generated code to replace fabric8-fabric8 specific annotations with annotations that might be usedeful for my project (e.g. javax.validation)?

@manusa manusa added this to the 6.2.0 milestone Sep 14, 2022
@andreaTP
Copy link
Member

worth adding a note on the the breaking changes list

I agree, just want to note that the Java-Generator is still in "preview", so breaking changes are fine there.
Maybe 6.2 is a good candidate to get the Java Generator out of the Preview?

Regarding your additional question, we should come up with something for sure ... 😓 I have a couple of half-baked ideas in mind but am not convinced yet, open to suggestions!

@fabiobrz
Copy link
Contributor Author

I think it would be worth adding a note on the the breaking changes list, since some users who might be using the Java generator, rely on the javax validation annotations that are no longer added.

Hi @manusa - thanks for your feedback, will do.

This also raises the question: as a developer relying on the java generator, how can I further transform the generated code to replace fabric8-fabric8 specific annotations with annotations that might be usedeful for my project (e.g. javax.validation)?

I've been discussing this shortly with @andreaTP as well last week.
Yesterday I was thinking about it and I came to the conclusion that maybe we'd want to create one issue for supporting the generation of validation constraints annotations per each framework type.

This would allow for them to be worked on asynchronously and for the support to different framework to be released in steps.
Maybe we could start with supporting the javax.validation.* as a PoC, in order to evaluate whether the proposal is actually fitting.

WDYT? CC @andreaTP

@andreaTP
Copy link
Member

for supporting the generation of validation constraints annotations per each framework type

This is an interesting idea, but I'm afraid to add this complexity to the core java-generator project.

I'm thinking about adding the possibility to arbitrarily customize the generated code before emitting it, offering a Visitor or Traverse pattern that can perform generic post-processing.

On top of that mechanism, we can implement separate plugins for each of the features we want to support, and they can possibly live in separate repositories/projects.

Note: the described mechanism might be used as a foundation to implement optimizations, such as class deduplication and similar.

@fabiobrz
Copy link
Contributor Author

for supporting the generation of validation constraints annotations per each framework type

This is an interesting idea, but I'm afraid to add this complexity to the core java-generator project.

I'm thinking about adding the possibility to arbitrarily customize the generated code before emitting it, offering a Visitor or Traverse pattern that can perform generic post-processing.

On top of that mechanism, we can implement separate plugins for each of the features we want to support, and they can possibly live in separate repositories/projects.

Note: the described mechanism might be used as a foundation to implement optimizations, such as class deduplication and similar.

Thanks @andreaTP!
+1 on loading the implementation dynamically, i.e. prepare the foundation for plugins. It makes sense to me and I actually thought of it as well, in relation to the complexity increase for the core module.

@fabiobrz fabiobrz force-pushed the java-gen.support-validation-annotations branch from 7d9040f to 13bbb10 Compare September 14, 2022 09:54
@fabiobrz fabiobrz closed this Sep 14, 2022
@fabiobrz fabiobrz reopened this Sep 14, 2022
@fabiobrz
Copy link
Contributor Author

fabiobrz commented Sep 14, 2022

I think it would be worth adding a note on the the breaking changes list

Done, should be ready fr merge as soon as CI checks pass.

FYI @manusa

@sonarcloud
Copy link

sonarcloud bot commented Sep 14, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@andreaTP andreaTP merged commit 7519e1c into fabric8io:master Sep 15, 2022
@andreaTP
Copy link
Member

Thanks!

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.

[java-gen] - Generated Java model should include new specific annotations (min, max, etc.)
3 participants