Baseline for a Java generator from CRD

[andreaTP]

Description

As discussed here I prepared a baseline for the development of the feature requested in #3642
If you prefer me to target a branch other than master just tell me 🙂

This requires some more work to consider it "shippable", but, on top of my previous POC I have added the following:

• Back-ported the codebase to Java 8
• Added a little Picocli CLI to use the tool from command line

cc. @metacosm @manusa @rohanKanojia

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

[centos-ci]

Can one of the admins verify this patch?

[metacosm]

The approach looks sound and covered by tests. Made some comments but there are more about code style than flaws that'd prevent this from getting merged. Some documentation on how to use the generator would help but if it covers your needs, I don't see why it shouldn't be merged.

[andreaTP]

@metacosm I believe I addressed all of your notes, and improved a little even the checks from Sonar. 😄

ready for next round!

[metacosm]

LGTM

[fabiobrz]

Hi @andreaTP - maybe this is a silly question, but to be honest I couldn't have a proper look to the sources, yet, so I might be missing something.

At a quick glance I noticed a test for a keycloak CRD, which includes descriptions for .spec properties.

Is it the desired behavior to have the relevant approval test not expecting for any comment on class or accessors? i.e. would it be reasonable to expect for Javadoc to be generated for getters/setters from properties description as well?

BTW - about accessors, has lombok been evaluated for generated sources?

[andreaTP]

Hi @fabiobrz ,
please consider that this is a baseline and it needs more work for sure.

I was actually adding support for JsonPropertyDescription today, I believe that this is interesting for you:
63c1bf1

Not sure about javaparser support for generating JavaDocs and/or how this should work, I encourage you to open an issue and we can speak there about.

about accessors

Currently the generated classes are annotated with sundrio and lombok e.g.: https://github.com/fabric8io/kubernetes-client/pull/3693/files#diff-06bcc522c01389840e7769e46000baa87a0aa5ae97ecf5ab873af6d8d3b5b25eR12-R18

[fabiobrz]

Hi @fabiobrz , please consider that this is a baseline and it needs more work for sure.

Sure, makes sense. I was just wondering whether it is going to be evaluated (and merged once done) as a baseline, e.g.: like a tech preview of a feature that will be completed in the future, or whether this will be considered as a draft, which I assume it shall not, since well... it is not marked as Draft :)

I was actually adding support for JsonPropertyDescription today, I believe that this is interesting for you: 63c1bf1

That is ok, meaning that OpenAPI generators would be able to restore the description from the Java model, which is ok. But IIUC, it is not about Javadoc ending up in the generated model.

Not sure about javaparser support for generating JavaDocs and/or how this should work, I encourage you to open an issue and we can speak there about.

Sure, thanks. BTW - looking at the sources, Javadoc comments should be supported, e.g. for fields: https://github.com/javaparser/javaparser/blob/master/javaparser-core/src/main/java/com/github/javaparser/ast/body/FieldDeclaration.java#L61

[andreaTP]

The agreed approach is to use this baseline, as you said, as a "tech preview that will be completed in the future".

Especially in order to receive proper PR reviews for the upcoming development/bug-fixes/features implementation.

[metacosm]

Yes, it's better to proceed incrementally than keep piling on an already big PR.

[fabiobrz]

Hi @andreaTP - since I looked into the code a bit yesterday, I thought I could provide an additional review.
Basically I dropped just minor comments but feel free to let me know what you think, if you think it could be useful.

[fabiobrz]

What about having properties for all the literal versions that have been used both in here and in the CLI POM?
Maybe even a <dependencyManagment> section at the parent (i.e. java-generator) level would do. WDYT?

[andreaTP]

In this sub-module, especially for testing, we are using libraries that are only used here ... I don't have a strong opinion but, I think that this can be fixed as soon as we share more (e.g. with the crd-generator)

[fabiobrz]

At least IMHO it should definitely be logged.

[andreaTP]

I have the code to remove the corresponding if along with the warning above, this should not happen at all.

[fabiobrz]

As stated previously, it is ok to me if this will be tracked as a future improvement, thanks!

[andreaTP]

@fabiobrz I targeted most of your comments, please, bear in mind that this is a "Baseline", I'm trying hard to not pile up more code/changes on this already reviewed PR.

I'll be happy to catch up (and/or review contributions ❤️ ) after this one gets in 🙂 .

[fabiobrz]

@fabiobrz I targeted most of your comments, please, bear in mind that this is a "Baseline", I'm trying hard to not pile up more code/changes on this already reviewed PR.

I'll be happy to catch up (and/or review contributions heart ) after this one gets in slightly_smiling_face .

Fully agree @andreaTP, I am happy with how you addressed my comments, thanks!

[sunix]

For the little Picocli CLI, What about using Quarkus ?

• benefit from the dev mode and continuous testing
• works well with picocli
• has native mode that could be a good option for a cli

[andreaTP]

@sunix I have no objections and I see the advantages, but I do believe that the Quarkus integration (along with CI to release the binaries etc. etc.) should be done in a separate PR as this one is already really big.

[andreaTP]

@manusa I really need your final review here to, eventually, sign-off this PR 🙏

• The CI is(mostly) green (the single failure is not related with these changes).
• I have answered most of the comments from the various reviewers, style has been improved upon request but no major issues have been found, so far, in the approach and design.
• The only outstanding issue(IMHO) is this but I'm holding off to fix it since I would like to discuss the pro/cons of a solution before merging it(I have a proposal ready to be PRd on top).
• I'm starting to be extremely reluctant to make any change to avoid follow up requests for additional changes.
• We are observing rising interest from the community for such feature.

I'm available and happy to facilitate this review in any possible way (pairing etc. etc.).

[OneCricketeer]

FWIW, I was able to compile and run this against a go-generated CRD we're using, and it looked okay at a glance 👍🏼 Haven't copied over the classes to the actual project, though.

[manusa]

Trying to test now, I'm getting errors with multi-document YAMLs. I understand this feature is not implemented yet.
We should probably consider this for the next iteration. I take that a common scenario will be to download a CRDs YAML file from a typical GH release and parse it with the tool.

[manusa]

A test with cert-manager.crds.yaml.txt produced invalid code.

Apparently enums can have spaces 😳

Maybe for this case, things can be simplified when dealing with enums by using @JsonValue for serialization and a customized @JsonCreator for deserialization

[andreaTP]

@manusa thanks for the review!!!

I'm getting errors with multi-document YAMLs. I understand this feature is not implemented yet.

yup, let's implement this on another iteration, should be easy but there is already quite a lot here already.

A test with cert-manager.crds.yaml.txt produced invalid code.
Apparently enums can have spaces 😳

Not only spaces but also / 🤦 , anyhow, for this PR:

• incorporated cert-manager in the compilation test (at least it produces valid Java code now)
• the deserialization of enums, in this case and when hitting the "degraded" flow (e.g. with numbers), is going to break,
  I do believe that we can track this in an issue and fix this properly in another iteration. wdyt?

Should be ready for another round! 🚀

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
7 Code Smells

88.6% Coverage
0.0% Duplication

[manusa]

Not only spaces but also / facepalm , anyhow, for this PR:

* incorporated `cert-manager` in the compilation test (at least it produces valid Java code now)

* the deserialization of enums, in this case and when hitting the "degraded" flow (e.g. with numbers), is going to break,
  I do believe that we can track this in an issue and fix this properly in another iteration. wdyt?

Sorry for being so picky, but I understand that the enum behavior is not OK. The code is compilable now, but if used, it will generate invalid resources.

I think that it might be better to use enums only if we are able to parse them.

For example, for the cert-manager, instead of generating a field private java.util.List<Usages> usages;, since the enum values have been sanitized and when serialized won't be OK, maybe just create a private java.util.List<String> usages;.
Which on the other hand, is what we are currently doing with our go generators.

Probably this would also "fix" the issue with the numbers.

[manusa]

As agreed, we merge this so that we can work iteratively on the current known limitations.
Also the current GitHub interface to review has become unstable.

[OneCricketeer]

Hello all, I added a PR to create a Docker image from this package! #3821

[OneCricketeer]

I just noticed that I don't think default fields are properly represented. Is this work being tracked anywhere?

Go operator type

  // +kubebuilder:default=1
  Replicas  int32                       `json:"replicaCount,omitempty"`

YAML CRD

replicaCount:
  default: 1
  format: int32
  type: integer

Generated Java

    @JsonProperty("replicas")
    private Integer replicas;

[andreaTP]

@OneCricketeer correct, default value is not taken in account in the java-generator (IIRC it doesn't have a counterpart in the crd-generator too).

Do you mind to open an issue for this?
I would also appreciate help deciding the encoding of it.
E.g. one option might be to use @JsonSetter(nulls=Nulls.SKIP) with an appropriate assignment and value.

[OneCricketeer]

Created #3869 as well as #3870

[andreaTP]

Thanks @OneCricketeer , appreciated!
And, please, consider contributing 🙂