[EPIC] CRD Generator v2

[manusa]

Description

Original discussion: #5942

As part of the new v7.0.0 release we need to make the CRD Generator (especially its API) as complete as possible to avoid any breaking changes in future v7 minor version releases.

Tasks

• Mark crd spec as required #3096
  Find a way to allow for @NonNull@Required to work on the method level and allow user to extend the getSpec method and customize it.
• Expose additional options and a main method for crd generator v2 #5950
• Align crd generator v1 and v2 output #5951
• Enable parallel processing with crd-generator v2 #5952
• CRDGenerator v2: Maven Plugin #5944
• CRDGenerator v2: Gradle Plugin #5945
• CRDGenerator v2: CLI Application #5958
• CRDGenerator: Selectable fields #6392
• CRDGenerator: Allow to configure conversion webhooks #5794
• CRDGenerator: Allow to configure categories #5795
• CRDGenerator: Add support for size constraints #5836
• CRDGenerator: Add support for exclusiveMinimum/exclusiveMaximum #5868
• CRDGenerator: Change format in @PrinterColumn to enum #6455
• Adapt CRD Generator v2 to new approach to KubernetesResource #6335
• CRDGenertor: Deprecate v1 #6673

[manusa]

Hi @baloo42,
Once the pending PRs for the CRDGenerator are merged I will probably start with the release process of v7.0.0.
Is there something else that we're missing that should go into v7? I'm talking about any breaking change that will be hard to introduce in any subsequent 7.x version release

[baloo42]

Ok, let's focus on stabilizing 7.0.0 from now on. My planned contributions can wait until the merge window for the next release is open.

In my opinion we should at least discuss the following potential candidates of breaking changes:

Mark spec always as required

This change was initially discussed in terms of introducing new interfaces e.g. HasSpec, HasStatus to replace CustomResource class.

If the introduction/change of those interfaces must be postponed we should talk at least of hardcoding this into the CRD-Generator. Marking the spec always as required can be easily implemented if hardcoded, it just needs some work on fixing most of the tests.

Can you provide an update on the plans regarding the CustomResource class?

If we introduce this, it would be definitly a breaking change.

#3096

Change of value type in @Min / @Max

At the moment those annotations use double as value type. Is this really safe and lossless?
Other comparable annotations from openapi or jakarta validations have other strategies in place to avoid rounding errors.

If we need to change the type, this would be a breaking change.

Mentioned in PR #6447 (comment).

#5868 itself might also contain some breaking changes, but this is already listed in your list.

@ValidationRule detection

At the moment @ValidationRule will be detected and merged together for a lot cases. Maybe this was too much and this is the chance to reduce some cases.

Example:
If a field and the corresponding getter have @ValidationRules, then all rules are merged and end up in the resulting CRD.

Should we stay to this behaviour or should we either pick this annotation from the field or the getter similar to other annotations.

#5942 (comment)
https://github.com/fabric8io/kubernetes-client/blob/main/crd-generator/api-v2/src/main/java/io/fabric8/crdv2/generator/AbstractJsonSchema.java#L201

Generated names in @PrinterColumn

The generated names for printercolumns are capitalized to be backwards compatible. I personally don't like this. The printer column name should be equal to the field name. If a user doesn't like this he can override the name anyways by using the annotations. I prefer less modifications under the hood.

Because the @PrinterColumn is probably the most used annotation, a change would produce a lot noise.

Merge the list annotation into the repeating annotation

We are using at the moment seperate files to define repeating annotations. I think it would be better to rename the list annotation to List and move it to into the repeating annotation.

Example:

@Target({ ElementType.TYPE })
@Retention(RetentionPolicy.RUNTIME)
@Repeatable(AdditionalSelectableField.List.class)
public @interface AdditionalSelectableField {

  String jsonPath();

  @Retention(RetentionPolicy.RUNTIME)
  @Target(ElementType.TYPE)
  @interface List {
    AdditionalSelectableField[] value();
  }
}

---

Additional notes:

• Expose additional options and a main method for crd generator v2 #5950 is in my opinion already implemented.
• CRDGenerator: Allow to configure conversion webhooks #5794 does not require a breaking change and can be therefore postponed.
• CRDGenerator v2: Gradle Plugin #5945 would be nice, but it's technically not a breaking nor a required change, so it can be postponed.

[manusa]

In general, v2 should be a seamless transition from v1, users should be able to switch from one to the other without incurring in many changes.

If this has not been done yet, v1 should be released as deprecated (for removal) in v7.0.0. This will allow for users to move to the new generator version gradually and fallback in case bugs are found in the new implementation.
For this we can add a simple warning in the annotation processor that states the deprecation and suggests to move to the new generator.

Eventually we will remove v1 in a subsequent v7 minor/y version once everything is stable enough.

Now let me go point by point:

• Mark spec always as required
  I'm going to investigate this now. The idea is to make the change in case I find a way for users to opt-out of this behavior. If not, we should not move forward with this.
  It's rare, but some users might have created custom resources without spec of status fields. Since the transition should be seamless (or almost seamless) we can't impose this without providing a way out.
  edit: After further investigation, I don't think we should move forward with this. Users can easily opt-in for the behavior by overriding the getSpec or getStatus methods and annotating them with @Required (just like every other field).
• Change of value type in @Min / @Max
  I think that what you've done in feat(crd-generator): Add support for more validation constraints #6447 is mostly backwards compatible. For the cases it's not (String, etc.), it's a very easy fix and even the approval tests can be created with support for both versions of the generator.
  It feels wrong that we are using double, Are there any plans or thoughts to tackle this?
  I'm not sure if this is actually problematic for the regular use cases.
  In any case, this can be easily covered in the future by adding additional fields to the annotation to express the value in the intended target type.
• Generated names in @PrinterColumn
  This goes against the initial principle of making the transition as seamless as possible.
  Given the time constraints, I'd rather see this (if we want to tackle it) in a future v8 (we can also discuss about major version timelines in the future, but the cadence/cycle should be shorter and more predictable).
• Merge the list annotation into the repeating annotation
  I guess this can also be implemented in a non-breaking manner in the future.

So, all things considered, I think that the only remaining point is to analyze if #3096 can be covered or not and merging #6447.

Once that is dealt with, we can proceed with the release.

Is this correct, or did I miss something?
I'm not sure if I mentioned this, but my priority right now is releasing v7.0.0, if possible, in the next days.

/cc @andreaTP @metacosm @shawkins

[baloo42]

In general, v2 should be a seamless transition from v1, users should be able to switch from one to the other without incurring in many changes.

Sorry, I was not aware that you want to avoid any breaking change at all. I thought it's acceptable to introduce some breaking changes to cleanup things.

I'm fine with all your comments. Thanks for clarification. Will work now on #6447 to polish the current state.

[manusa]

Sorry, I was not aware that you want to avoid any breaking change at all. I thought it's acceptable to introduce some breaking changes to cleanup things.

Not any at all, but just the few inevitable to allow for the new features.

Maybe this is not exactly what was discussed in the past.

At some point in v6 we introduced the new CRD generator in preview. Some users migrated to the new one, but the v1 CRD generator wasn't deprecated yet. Since we're deprecating it in v7, we need to allow for an easy transition for users. IMO it will be better if v2 is just a drop-in replacement (almost) that requires users to only change their project configuration.

My expectation is that our release cadence improves in the future and is more predictable, so anyway, introducing any breakages shouldn't involve such a long process.