feat: add getPlural method on GroupVersionKind #2515

metacosm · 2024-08-23T16:51:53Z

Signed-off-by: Chris Laprun [email protected]

...or-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/GroupVersionKind.java

csviri · 2024-08-26T07:59:14Z

...or-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/GroupVersionKind.java

 import io.fabric8.kubernetes.api.model.HasMetadata;

 public class GroupVersionKind {
  private final String group;
  private final String version;
  private final String kind;
+  private final String plural;


What is the use case of plural?

it is not present on go side either afaik. And usually just used from places like kubectl

While the plural can most of the time be inferred from the kind, it could be useful to know the precise plural version associated with resources identified by this GVK, if it is known. In particular, this would be useful for the linked PR (see: https://github.com/quarkiverse/quarkus-operator-sdk/pull/931/files#diff-b780cb662fc5c0a824655ac29ced7dca9efd35a33f771cd3f3a84a46f21b7cebR97 in particular).
The plural form is used in many spots, this is actually what is used to identify the REST endpoint to access when dealing with resources so knowing its precise form (if known) could be useful for pretty much anything that deals with RBACs.
To be clear, this is a convenience method which should be transparent to people not using it.

Sorry did not see the link. My only concern is, that in JOSDK scope this has no usage whatsoever, that might lead to confusion. Currently it has very well defined scope. So in that regard basically we have leaking requirements from QOSDK to JOSDK, shouldn't be rather a GroupVersionKindPlural class (extending GVK) created in QOSDK, so we have nice separation of concerns between the projects?!

This isn't a requirement but this is something (what is the plural form?) that someone might want to know when given a GVK. It makes sense on its own, imo. We could rename the class but I think it makes sense as-is, not to mention that keeping the same name would avoid breaking the API and thus requiring waiting for a major version for this feature, while I would like to get that released in a patch release quickly if possible.

Look this isn't nitpicking, this is a central technical class that I want to keep as clean as possible. GVK plays a important role in dynamic APIs, check the glue operator: https://github.com/csviri/kubernetes-glue-operator basically all the keys in caches and stuff are built on this. This is reasonable to assume from anyone that builds dynamic stuff. So we really want to have this as clean as possible.

You make an argument against an extension to a class that could benefit other projects based on the fact that another project uses that class. That seems rather self-serving. There are no more reason to "help" the Glue operator by keeping GVK "clean" than helping other projects by adding the plural version here.
Moreover, how exactly is adding the plural form (which is central to how resources are handled by kubernetes) "dirtying" the GVK class? It's not like it's adding something that's completely unrelated: plural is derived from kind so it makes sense to have it there, especially when the derivation is not trivial and can't only be inferred from the kind. Classes that use GVK lose nothing by having GVK provide a plural form as well.

It is very simple to create a GroupVersionKindPlural in QOSDK that simply extends GroupVersionKind and the boundaries will be very clean.

That is not true because some information might be lost by not providing the plural form. It might actually make sense to make getPlural return Optional<String> instead to convey that information.

If anything, you could make a point that GVK shouldn't be exposed at all outside of JOSDK. You could also make a point that JOSDK should have used the GVK class from the Fabric8 client instead of creating its own. JOSDK created its own version and exposed it outside of it, that's OK and it's used by other projects, that's also OK. I'm arguing that this class could be made even more useful to outside projects by making the plural form available which JOSDK is often able to compute properly, which might not be the case for external projects that might lack the information JOSDK has.

The goal of a framework is to be useful to external projects, not adhere to some weird concept of "purity". Having plural on GVK increases integration opportunities by exposing information that JOSDK is uniquely positioned to compute correctly.

What I wanted to point is that the scope of this class now is to identify a Kubernetes object type. That is done by using Group, Version, Kind in Kubernetes. Plural plays a role on completely different places has nothing to do with this goal.

It is completely fine to use GVK for this purpose outside of the project. But by adding the plural has nothing to do with this function (thus identifying a type). That is what I mean clean. Now you want to add plural, because it fits for some functionality related to RBAC generation in QOSDK. Tha breaks the definition of goal of this class.

I'm fine with having GroupVersionKindPlural that extends GroupVersionKind in JOSDK, saying that this is a class that helps in other places (That would make clear separation of concerns.). But not by adding plural into GroupVersionKind directly.

see in Kubernetes: https://pkg.go.dev/k8s.io/apimachinery/pkg/runtime/schema#GroupVersionKind

note also the other types in that package: https://pkg.go.dev/k8s.io/apimachinery/pkg/runtime/schema#pkg-types
with different purpose.

csviri

Was thinking more about this, to my understanding (correct me if wrong):

We need GroupVersionKindPlural only to generate RBAC for GenericKubernetesDependentResource (GKDR) in Quarkus.
For example for InformerEventSource if we don't use DR we never need the GVKP only GVK.
For GKDR we need only (user will specify) if the Plural is special non computable way.

Therefore thinking this PR is good, but:

we should not deprecate GVK, it should be still used for Informer, also for GKDR if no special plural is needed.
What would be nice is to add an additional constructor to GKDR that takes GVKP. So we make it explicit in the code that is used there.

csviri · 2024-08-29T07:26:33Z

...sresource/generickubernetesdependentresourcemanaged/ConfigMapGenericKubernetesDependent.java

@@ -24,7 +25,7 @@ public class ConfigMapGenericKubernetesDependent extends
  public static final String KEY = "key";

  public ConfigMapGenericKubernetesDependent() {
-    super(new GroupVersionKind("", VERSION, KIND));


The point here was to not use the class, but demonstrate how to do it classless or "dynamically". Would be nicer with a dedicated custom resource for sure. I just used config map. Pls revert this.

csviri · 2024-08-29T07:26:44Z

...rnetesresource/generickubernetesdependentstandalone/ConfigMapGenericKubernetesDependent.java

@@ -24,7 +25,7 @@ public class ConfigMapGenericKubernetesDependent extends
  public static final String KEY = "key";

  public ConfigMapGenericKubernetesDependent() {
-    super(new GroupVersionKind("", VERSION, KIND));
+    super(GroupVersionKind.gvkFor(ConfigMap.class));


Same as above.

metacosm · 2024-08-29T07:35:35Z

Therefore thinking this PR is good, but:

* we should not deprecate GVK, it should be still used for Informer, also for GKDR if no special plural is needed.

It is deprecated so that it is not used outside of JOSDK anymore. The idea is to expose only GVKP to external users, not GVK since it is only used internally so users will only deal with GVKP, which should only be needed outside of JOSDK via GKDR.

* What would be nice is to add an additional constructor to GKDR that takes GVKP. So we make it explicit in the code that is used there.

There is one already.

csviri · 2024-08-29T07:58:03Z

There is one already.

Sorry missed that. Great.

csviri · 2024-08-29T07:59:14Z

It is deprecated so that it is not used outside of JOSDK anymore. The idea is to expose only GVKP to external users, not GVK since it is only used internally so users will only deal with GVKP, which should only be needed outside of JOSDK via GKDR.

My point it is that is should not be deprecated, should not be only internal. Either we hide both on APIs, or neither. Since for example for InformerEventSource initialization we should not force GVKP. For that should be GVK available.

csviri · 2024-08-29T08:02:03Z

It is deprecated so that it is not used outside of JOSDK anymore. The idea is to expose only GVKP to external users, not GVK since it is only used internally so users will only deal with GVKP, which should only be needed outside of JOSDK via GKDR.

My point it is that is should not be deprecated, should not be only internal. Either we hide both on APIs, or neither. Since for example for InformerEventSource initialization we should not force GVKP. For that should be GVK available.

In both cases this could be hidden by just accepting. 3 respectively 4 strings instead of GVK / GVKP in constructors, builder methods.

Signed-off-by: Chris Laprun <[email protected]>

…DependentResource Signed-off-by: Chris Laprun <[email protected]>

Signed-off-by: Chris Laprun <[email protected]>

metacosm · 2024-08-29T14:47:20Z

I believe that all the comments should now be addressed.

csviri

LGTM, great, thank you!!

metacosm self-assigned this Aug 23, 2024

metacosm requested review from csviri and xstefank August 23, 2024 16:51

openshift-ci bot requested review from adam-sandor and andreaTP August 23, 2024 16:51

metacosm mentioned this pull request Aug 23, 2024

Support typeless kubernetes resources when generating manifests quarkiverse/quarkus-operator-sdk#931

Merged

csviri requested changes Aug 26, 2024

View reviewed changes

xstefank approved these changes Aug 26, 2024

View reviewed changes

metacosm force-pushed the improve-gvk branch from acc4074 to 67693eb Compare August 28, 2024 12:05

csviri requested changes Aug 29, 2024

View reviewed changes

csviri reviewed Aug 29, 2024

View reviewed changes

metacosm force-pushed the improve-gvk branch from 49da6f0 to 042ed26 Compare August 29, 2024 14:44

metacosm added 4 commits August 29, 2024 16:46

feat: add getPlural method on GroupVersionKind

18e337d

Signed-off-by: Chris Laprun <[email protected]>

fix: make getPlural return Optional to show whether plural is known

313a5f0

Signed-off-by: Chris Laprun <[email protected]>

refactor: add GroupVersionKindPlural class, used by GenericKubernetes…

240fdba

…DependentResource Signed-off-by: Chris Laprun <[email protected]>

refactor: clean up, cache already resolved GVKs

4756be3

Signed-off-by: Chris Laprun <[email protected]>

metacosm force-pushed the improve-gvk branch from 042ed26 to 4756be3 Compare August 29, 2024 14:46

metacosm requested a review from csviri August 29, 2024 14:47

csviri approved these changes Aug 29, 2024

View reviewed changes

metacosm merged commit 3588780 into main Aug 29, 2024
20 checks passed

metacosm deleted the improve-gvk branch August 29, 2024 15:58

metacosm mentioned this pull request Aug 30, 2024

Expose method that computes the default plural form for a given kind fabric8io/kubernetes-client#6313

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add getPlural method on GroupVersionKind #2515

feat: add getPlural method on GroupVersionKind #2515

metacosm commented Aug 23, 2024

csviri Aug 26, 2024

csviri Aug 26, 2024 •

edited

Loading

metacosm Aug 26, 2024

csviri Aug 26, 2024 •

edited

Loading

metacosm Aug 26, 2024

metacosm Aug 26, 2024

csviri Aug 26, 2024 •

edited

Loading

csviri Aug 26, 2024 •

edited

Loading

csviri Aug 26, 2024

csviri Aug 26, 2024

csviri left a comment

csviri Aug 29, 2024

csviri Aug 29, 2024

metacosm commented Aug 29, 2024

csviri commented Aug 29, 2024

csviri commented Aug 29, 2024 •

edited

Loading

csviri commented Aug 29, 2024

metacosm commented Aug 29, 2024

csviri left a comment

feat: add getPlural method on GroupVersionKind #2515

feat: add getPlural method on GroupVersionKind #2515

Conversation

metacosm commented Aug 23, 2024

Choose a reason for hiding this comment

csviri Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csviri Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csviri Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

csviri Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csviri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metacosm commented Aug 29, 2024

csviri commented Aug 29, 2024

csviri commented Aug 29, 2024 • edited Loading

csviri commented Aug 29, 2024

metacosm commented Aug 29, 2024

csviri left a comment

Choose a reason for hiding this comment

csviri Aug 26, 2024 •

edited

Loading

csviri Aug 26, 2024 •

edited

Loading

csviri Aug 26, 2024 •

edited

Loading

csviri Aug 26, 2024 •

edited

Loading

csviri commented Aug 29, 2024 •

edited

Loading