Baseline for a Java generator from CRD - #3693

Can one of the admins verify this patch?

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

ready for next round!

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?

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

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

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.

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

@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 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!

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

@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.

@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.).

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.

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.

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

@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! 🚀

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
7 Code Smells

88.6% Coverage
0.0% Duplication

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.

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.

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

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;

@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.

Created #3869 as well as #3870

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