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

Move all CRD-related annotations (java-2-crd / crd-2-java) to a single package #5429

Closed
manusa opened this issue Sep 6, 2023 · 7 comments · Fixed by #5547
Closed

Move all CRD-related annotations (java-2-crd / crd-2-java) to a single package #5429

manusa opened this issue Sep 6, 2023 · 7 comments · Fixed by #5547
Milestone

Comments

@manusa
Copy link
Member

manusa commented Sep 6, 2023

Description

Ensure that all of the annotations on which the CRD-2-java and java-2-CRD modules rely on are on a single API package.

@manusa manusa added this to the 6.9.0 milestone Sep 6, 2023
@andreaTP
Copy link
Member

andreaTP commented Sep 8, 2023

@manusa thanks for filing this issue, there is a rationale behind the current split:

  • shared annotations can be used by both crd-2-java and java-2-crd sub-modules
  • java-2-crd annotations are usable exclusively by the java-2-crd generator since they do not make sense "the other way around"

More specifically:

  • Annotations: are ignored by the crd-2-java generator
  • Labels: are ignored by the crd-2-java generator
  • PreserveUnknownFields: is performed automatically (or by configuration if this PR gets merged), in the crd-2-java is encoded using the additionalProperties field instead of an annotation
  • PrinterColumn: is ignored by the crd-2-java generator
  • SchemaFrom / SchemaSwap / SchemaSwaps: are tricks to artificially "patch" the class graph

@shawkins
Copy link
Contributor

shawkins commented Sep 8, 2023

java-2-crd annotations are usable exclusively by the java-2-crd generator since they do not make sense "the other way around"

They can be in a different package, but they should both be in the generator-annotations module. If you don't know to scope crd-generator-api as provided, you'll end with extra stuff in your runtime classpath.

@andreaTP
Copy link
Member

andreaTP commented Sep 8, 2023

If you don't know to scope crd-generator-api as provided, you'll end with extra stuff in your runtime classpath.

The crd-generator is exclusively used at compile-time and should be marked as provided always, there is not much, other than documentation, we can do to prevent this kind of misuse.

The rationale behind the current separation is that:

  • using the crd-generator it's already on the classpath and you have access to all of the annotations
  • using the java-generator you only get the subset that you will effectively use

That said, if there is a general consensus to move everything to one sub-module I won't oppose it.
Reminder that we would need to roll this change in 2 minor release cycles:

  1. deprecate the current annotations and introduce the new one
  2. remove the deprecated annotations

@shawkins
Copy link
Contributor

shawkins commented Sep 8, 2023

The crd-generator is exclusively used at compile-time and should be marked as provided always, there is not much, other than documentation, we can do to prevent this kind of misuse.

Documentation would be nice. But also consider the annotations are marked as runtime retention. In theory a user could expect to reflectively make decisions based upon them if they include the dependency at the compile scope - but again would need to exclude the unnecessary transitive dependencies.

Reminder that we would need to roll this change in 2 minor release cycles:

Only if they are repackaged. Just moving them to a different jar, that is likely already included in the classpath shouldn't be a problem.

@andreaTP
Copy link
Member

andreaTP commented Sep 8, 2023

Documentation would be nice. But also consider the annotations are marked as runtime retention. In theory a user could expect to reflectively make decisions based upon them if they include the dependency at the compile scope - but again would need to exclude the unnecessary transitive dependencies.

I'm not sure we really want this to happen anytime soon.

Only if they are repackaged. Just moving them to a different jar, that is likely already included in the classpath shouldn't be a problem.

Here I disagree, moving to a different artifact IS a breaking change, If there are no compelling reasons to rush this I strongly suggest going through the 2 cycles.

@shawkins
Copy link
Contributor

shawkins commented Sep 8, 2023

Here I disagree, moving to a different artifact IS a breaking change,

Under what scenario would someone have a depedency, either provided or compile, on crd-generator-api and specifically exclude the generator-annoations transitive dependency? That seems quite unlikely.

If there are no compelling reasons to rush this I strongly suggest going through the 2 cycles.

That will require a repacking. Due to java module restrictions we don't want split packages.

@manusa
Copy link
Member Author

manusa commented Sep 18, 2023

We can discuss this internally on this week's sync meeting.

@manusa manusa modified the milestones: 6.9.0, 6.10.0 Oct 5, 2023
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Oct 25, 2023
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Oct 25, 2023
manusa pushed a commit to shawkins/kubernetes-client that referenced this issue Nov 13, 2023
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 a pull request may close this issue.

3 participants