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

HasMetadata should have convenience methods for annotations and labels #3625

Closed
shawkins opened this issue Nov 30, 2021 · 3 comments
Closed

Comments

@shawkins
Copy link
Contributor

Is your enhancement related to a problem? Please describe

Similar to #3600 if we are adding utility methods for owner references it would be good to have similar methods for labels and annotations.

Describe the solution you'd like

This would cover at least for getting:

resource.getLabel("key");
resource.getOrDefaultLabel("key", "default");
etc.

It could also include setting labels/annotations as well.

Describe alternatives you've considered

The code generation could be updated to use a default map for annotations and labels that ideally would also be omitted from serialization when empty. I'm not sure how involved this would be, so it seems easier to put new methods on HasMetadata.

Additional context

No response

@stale
Copy link

stale bot commented Feb 28, 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!

@manusa
Copy link
Member

manusa commented Mar 3, 2022

How about making some sort of convenience method in HasMetadata that delegates to GenericKubernetesResource#get.

Example:

  default <T> T get(Object... path) {
    return Serialization.jsonMapper().convertValue(this, GenericKubernetesResource.class).get(path);
  }
  default <T> T getOrDefault(T defaultValue, Object... path) {
    final T value = get(path);
    return value == null ? defaultValue : value;
  }

This would also require a minor change in GenericKubernetesResource to consider the metadata field (if (path[0].equals("metadata"))

@shawkins
Copy link
Contributor Author

shawkins commented Mar 5, 2022

This probably has a similar cost as a builder, which it seems expensive for just reads.

This would also require a minor change in GenericKubernetesResource to consider the metadata field (if (path[0].equals("metadata"))

More generally there would be some utility in having everything parsed as just abstract model (map of maps - the old generic), and have the ObjectMeta aspect just applied as a view. But I can see that doesn't work well with building logic - https://github.com/fabric8io/kubernetes-client/pull/3931/files#diff-68db8ad66510b2e36592ac1c814508958f9dfbc42ea19a3c2bcc2dda89305697R51 - and is also less performant for normal read access.

If it's not doable to have a default metadata, we at a could add HasMetadata methods - Optional getOptionalMetadata() and getOrCreateMetadata.
But even with a guarantee that getAnnotations returns non-null

resource.getOptionalMetadata().map(m -> m.getAnnotations().get("key")).orElse(null)

that's only a little better than what you'd have to do today

Optional.ofNullable(resource.getMetadata()).map(ObjectMeta::getAnnotations).map(m -> m.get("key")).orElse(null)

So I think we should at least add:

resource.getAnnotations().get("key")
resource.getLabels().get("key")

And document the behavior that it will auto create the objectmeta as needed. That's probably preferable to the direct methods mentioned in the description.

Then if we can also update the generation logic to make annotations and labels nominally non-null that will make their usage off of objectmeta straight-forward as well.

shawkins added a commit to shawkins/kubernetes-client that referenced this issue May 18, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue May 18, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue May 18, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue May 18, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue May 18, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue May 18, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue May 18, 2022
@manusa manusa closed this as completed in 92a67ad May 23, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue May 25, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue May 25, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue May 25, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue May 25, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue May 25, 2022
manusa pushed a commit that referenced this issue May 25, 2022
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

No branches or pull requests

2 participants