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

Use one resource annotation #4164

Open
shawkins opened this issue May 22, 2022 · 7 comments
Open

Use one resource annotation #4164

shawkins opened this issue May 22, 2022 · 7 comments

Comments

@shawkins
Copy link
Contributor

Is your enhancement related to a problem? Please describe

We have separate annotations for things like group, kind, plural, singular, and version.

Describe the solution you'd like

For those that don't auto-generate their model, it would be better if all of the single valued annotations could easily be combined to a single annotation:

Resource(group="x", kind="y" ...)

You could even include builder=Something.class, and list=Something.class - if we think that the conventions may not hold at some point.

Describe alternatives you've considered

No response

Additional context

No response

@manusa
Copy link
Member

manusa commented Jun 6, 2022

This would make the code look more organized, and reduce the code-base by a few hundred lines.

However, won't this break our current model backwards compatibility support (i.e. consuming a model from v5.9 in v5.12)?

@shawkins
Copy link
Contributor Author

shawkins commented Jun 6, 2022

However, won't this break our current model backwards compatibility support (i.e. consuming a model from v5.9 in v5.12)?

You'd have to keep and deprecate the old style for a couple of releases.

@shawkins
Copy link
Contributor Author

A full proposal would look like:

public @interface Resource {
  
  String group();
  
  String kind() default "";  // empty would default to class name
  
  String plural() default ""; // empty would default to plualize kind
  
  boolean namespaced() default true;
  
  String version();
}

This would also mean deprecating the Namespaced interface - it won't be needed once this annotation is required.

The only sticking point are how this meshes with the crd related annotations - in particular Version can accept storage and served for that purpose. The simplest thing is that you could just flatten these attributes into the Resource annotation as well. The advantages of doing this are:

  • is much more straight-forward for those starting with the object model
  • is more straight-forward for annotation scanning. As of now you need to scan for Version - as that's the only one that's effectively required
  • generated code will be a little simpler

On the other hand it will be a fair amount of work and introduce some confusion while both sets of annotations are supported.

On versions, an additional thought - for multi-version support from a single object model, we'd need to support something like:

client.resources(Something.class, "v2") - under the covers we'd create a ResourceDefinitionContext off of Something, but change the version to v2. I'm not sure if this is a common enough use case to add another entry method.

@stale
Copy link

stale bot commented Oct 14, 2022

This issue has been automatically marked as stale because it has not had any activity since 90 days. It will be closed if no further activity occurs within 7 days. Thank you for your contributions!

@stale stale bot added the status/stale label Oct 14, 2022
@stale stale bot closed this as completed Oct 21, 2022
@manusa manusa reopened this Nov 28, 2022
@manusa manusa added this to the 7.x milestone Nov 28, 2022
@manusa
Copy link
Member

manusa commented Nov 28, 2022

A full proposal would look like:

public @interface Resource {
  
  String group();
  
  String kind() default "";  // empty would default to class name
  
  String plural() default ""; // empty would default to plualize kind
  
  boolean namespaced() default true;
  
  String version();
}

This would also mean deprecating the Namespaced interface - it won't be needed once this annotation is required.

This proposal sounds good. However, I'd defer this changes to a future 7.x (next major) release where architectural breaking changes would fit better.

@manusa
Copy link
Member

manusa commented Mar 7, 2024

This is something we might want to consider for version 7.0

@metacosm
Copy link
Collaborator

One thing to consider here is that it would be nice to be able to determine whether a resource is namespaced or not without the annotation (as is the case at the moment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants