Add crd-generator and java-generator support for Format annotation

[matteriben]

Description

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

[andreaTP]

The CRD-generator changes are good to me(maybe @baloo42 is interested in reviewing).

I see the motivations for those changes but we should not emit SchemaFrom and/or SchemaSwap from the java-generator.
It's fine to generate the Format annotations though.

[andreaTP]

related #5881

[baloo42]

The implementation looks good to me regarding the CRD-Generator. It also fits to #5881. (My prototype looks the same but I hold it back because I prefer @Schema to avoid annotation explosion). But it's possible to implement both: If format is defined by @Format and @Schema, the format value from @Schema would win (which would break roundtrips again).

I only would suggest to make it more clear that format can be used for other formats than date-time.

Regarding java-generator: I'm not able to say something in depth because I'm not actively using it. But to me it looks like a breaking change for some users.

[baloo42]

Beside of the nitpick in the docs it looks now good to me regarding the CRD-Generator

[andreaTP]

Although I don't see much value for the java-generator part, I'm fine with those changes, thanks 👍

[manusa]

LGTM, thx!

[shawkins]

@manusa @andreaTP ideally we would not add support a custom Format annotation.

[baloo42]

@manusa @andreaTP ideally we would not add support a custom Format annotation.

@shawkins Does this mean you would prefer a general annotation like described in #5881?

[andreaTP]

ideally we would not add support a custom Format annotation.

I see the use case for this when using the CRD Generator, at the moment you cannot inject a custom format.
Is just another "free form string field" that accepts any value.

[shawkins]

I see the use case for this when using the CRD Generator, at the moment you cannot inject a custom format.
Is just another "free form string field" that accepts any value.

There is built-in support for the format within Jackson, which is now wired up into the prospective changes in my next state branch. There may be some usability limitations with that, so it's possible that a Format annotation will still make sense. If not we'll just deprecate it in 7 like some of the other annotations.

@matteriben and others - the current progress on what I'd like to see as the 7.0 state of the crd generator is at
https://github.com/fabric8io/kubernetes-client/compare/main...shawkins:kubernetes-client:iss5866-b?expand=1

Ideally we could start to minimize the 6.x version changes, or at least get a first class branch going for the next state so that it can be targeted with pull requests for support like this as well.

[baloo42]

I see the use case for this when using the CRD Generator, at the moment you cannot inject a custom format.
Is just another "free form string field" that accepts any value.

There is built-in support for the format within Jackson, which is now wired up into the prospective changes in my next state branch. There may be some usability limitations with that, so it's possible that a Format annotation will still make sense. If not we'll just deprecate it in 7 like some of the other annotations.

@matteriben and others - the current progress on what I'd like to see as the 7.0 state of the crd generator is at https://github.com/fabric8io/kubernetes-client/compare/main...shawkins:kubernetes-client:iss5866-b?expand=1

Ideally we could start to minimize the 6.x version changes, or at least get a first class branch going for the next state so that it can be targeted with pull requests for support like this as well.

After looking at the planned changes it doesn't make sense to me to add features / annotations to v6 of the CRD-Generator anymore. Any new feature would have to be replaced by a new implementation based on Jackson.

I also see, that the crd-generator-apt module will be removed. Do you plan a replacement?

Can I help you with v7 instead? e.g. with some feedback on comments or implementing some of the other missing features. I would suggest to create a crd-generator-v7-development branch in the kubernetes-client project.

[shawkins]

I also see, that the crd-generator-apt module will be removed. Do you plan a replacement?

Yes, we have discussed adding a maven plugin and a gradle plugin for non-quarkus / josdk based usage. Maybe something utilizing jandax for discovery?

Can I help you with v7 instead? e.g. with some feedback on comments or implementing some of the other missing features. I would suggest to create a crd-generator-v7-development branch in the kubernetes-client project.

Absolutely. I'll create that branch, and we can how it can be adapted to your needs.

I had just sent a message to other team members desribing the state of things, let me know what you may want to work on and we can sync up our efforts -

Where things stand:

• there are a couple of hacks, noted with TODOs, due to choices we made about where to pick up annotations from. Nothing too crazy.
• the usage of CustomResource has been made optional
• the core logic is no longer static. It's not quite wired all the way up to accept an ObjectMapper, but that shouldn't be too hard
• parallelism has not been wired back in. It will be straight-forward to add that back if needed (either separate ResolvingContexts, or make sure that it's thread-safe)
• there are some generation differences, there are three tests failing due to:
  • The required list was previously unordered, now it is.
  • The printer column type is taken directly from the property - but in the test it's expecting a string instead of an array type for one of the columns. What is the expectation here?
  • It now better understands the anygetter / setter logic, so more properties are marked as preserve unknown
• all sundrio has been removed including the apt module
  • the crd-generator/test tests have not yet been rewired to work. In the interim I could just manually perform generation.

Next steps:

• create a formal development branch? We'll probably want the flurry of work that is being done to target both 6.x and the next state.
• maven and gradle plugins
• PR to retarget the quarkus logic once we can allow for the objectmapper to be passed in

Related:

• change the default time related serialization to a string, rather than number / array

[shawkins]

@matteriben @baloo42 @andreaTP here's an upstream development branch https://github.com/fabric8io/kubernetes-client/tree/crd-generator-v7-development

[baloo42]

Thanks for the detailed answer. I think we should move the further discussion out of this PR:

#5942

[manusa]

So let's merge this one then??

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
96.0% Coverage on New Code
1.5% Duplication on New Code

See analysis details on SonarCloud

[manusa]

So let's merge this one then??

@shawkins, sorry, it was unclear but the question was specifically for you. It's unclear to me if this should be moved forward as is or not given your previous comments.

[shawkins]

So let's merge this one then??

@shawkins, sorry, it was unclear but the question was specifically for you. It's unclear to me if this should be moved forward as is or not given your previous comments.

Let's have @baloo42 and @matteriben check if their use cases are satisfied by the v2 logic already. If so, then we can close this pr. If not, then we'll probably want to retarget to v2, rather than introducing additional v1 support.

[matteriben]

Let's have @baloo42 and @matteriben check if their use cases are satisfied by the v2 logic already. If so, then we can close this pr. If not, then we'll probably want to retarget to v2, rather than introducing additional v1 support.

My use case does appear to be handled as desired in the v2 branch.

[manusa]

OK, so let's try to merge #5949 (#5948) first and then add the functionality on top

[baloo42]

After testing v2 it seems like the new implementation covers a lot more cases for format but unfortunatly not all.

Not covered:

• ipv4
• ipv6
• email
• mac
• ...

But is that worth to introduce an own annotation? Maybe we can cover that with a single @Schema annotation... (#5881)

[shawkins]

@baloo42 in addition to any general proposal, it would help to capture where each of those could have additional support in jackson, or with the v2 generator as well. Anywhere the type alone is sufficient, we should easily cover.

[baloo42]

@baloo42 in addition to any general proposal, it would help to capture where each of those could have additional support in jackson, or with the v2 generator as well. Anywhere the type alone is sufficient, we should easily cover.

If I understand you correct, you mean that e.g. Inet4Address could be mapped to:

type: string
format: ipv4

I think this can be a solution but we must be aware of that Jackson contains sometimes some overriding logic with a shape:

https://github.com/FasterXML/jackson-databind/blob/master/src/main/java/tools/jackson/databind/ser/jdk/InetAddressSerializer.java#L41

This might require some more code but I don't expect too much of such cases, so I'm fine with that strategy.

[shawkins]

I think this can be a solution but we must be aware of that Jackson contains sometimes some overriding logic with a shape:

Sure, ideally all of this should be considered in the serializer. The InetAddressSerializer should be overriding acceptJsonFormatVisitor and passing the relevant JsonValueFormat - this is what DateTimeSerializer is doing.

We can of course add handling on our side for it, but I'd prefer to push things upstream where possible.

[metacosm]

LGTM