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.

Sign up for GitHub

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

Jump to bottom

Load any Submit resources without mentioning kind types and api group/versions #3336 #3744

Open

vaidikcode wants to merge 5 commits into kubernetes-client:master from vaidikcode:master

vaidikcode commented Oct 17, 2024

#3336 As described I tried to follow your approach and created a method to load and submit resources and then passed it on to the Api handler
I don't know where to mention the use case example-

public static void main(String[] args) throws Exception {

String yamlContent = "apiVersion: v1\n"
        + "kind: Pod\n"
        + "metadata:\n"
        + "  name: my-pod\n";


Object createdResource = Yaml.loadAndSubmitResource(yamlContent);


System.out.println(createdResource);


          commit 1

b218eb4

Contributor

k8s-ci-robot commented Oct 17, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: vaidikcode
Once this PR has been reviewed and has the lgtm label, please assign yue9944882 for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested review from brendandburns and yue9944882

October 17, 2024 16:48

linux-foundation-easycla bot commented Oct 17, 2024 •

edited

Loading

The committers listed above are authorized under a signed CLA.

✅ login: vaidikcode / name: Vaidik (4a0dde2, f851639, b218eb4, 963b83e, 3c7f69c)

k8s-ci-robot added the cncf-cla: no label

Contributor

k8s-ci-robot commented Oct 17, 2024

Welcome @vaidikcode!

It looks like this is your first PR to kubernetes-client/java 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-client/java has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot added the size/L label

Member

yue9944882 commented Oct 17, 2024

@vaidikcode thank you for the contribution. the Yaml.java file where the change is proposed is designed/dedicated for yaml content [de]serialization. i think a better place to bring this enhancement in is the Kubectl related utilities under the dynamic package e.g.:

https://github.com/kubernetes-client/java/blob/master/util/src/main/java/io/kubernetes/client/util/generic/dynamic/Dynamics.java#L44-L54

We can add a short-cut util function under Dynamics probably named createFromYaml to achieve that functionality, wdyt?

Author

vaidikcode commented Oct 17, 2024

@yue9944882 Yes, I think it would be much better. Can you elaborate on how I can do this? Can you also tell me if I need to include an example code to use this method, and if yes, please tell me where to include the example code.

brendandburns reviewed

View reviewed changes

util/src/main/java/io/kubernetes/client/util/Yaml.java Outdated

-              import java.util.Map;
-              import java.util.Optional;
-              import java.util.Set;
+              import java.util.*;

Contributor

brendandburns Oct 18, 2024

Remove wildcard imports.

brendandburns reviewed

View reviewed changes

util/src/main/java/io/kubernetes/client/util/Yaml.java Outdated

+                 * @return An instantiation of the object.
+                 * @throws IOException If an error occurs while reading the YAML.
+                 */
+                public static Object loadAndSubmitResource(String content) throws Exception {

Contributor

brendandburns Oct 18, 2024

It's more flexible to load from a Reader rather than a String you can always create a StringReader

brendandburns reviewed

View reviewed changes

util/src/main/java/io/kubernetes/client/util/Yaml.java

+                  //loading yaml content as unstructured object
+                  Object unstructuredObject = Yaml.load(new StringReader(content));
+                  if (!(unstructuredObject instanceof Map)) {

Contributor

brendandburns Oct 18, 2024

fwiw, this will only work for single-instance YAML files, if people put multiple objects in a single file, it will fail. That's fine for a starting point, but you may want to document it somewhere.

brendandburns reviewed

View reviewed changes

util/src/main/java/io/kubernetes/client/util/Yaml.java Outdated

+                  Object unstructuredObject = Yaml.load(new StringReader(content));
+                  if (!(unstructuredObject instanceof Map)) {
+                    throw new IllegalArgumentException("Invalid YAML");

Contributor

brendandburns Oct 18, 2024

I think ParseException might be more appropriate? (here and below)

brendandburns reviewed

View reviewed changes

util/src/main/java/io/kubernetes/client/util/Yaml.java Outdated

+                  String version;
+                  // If apiVersion contains '/', then it means there is a specified group for it and we need to get that else it is a default group
+                  if (apiVersion.contains("/")) {

Contributor

brendandburns Oct 18, 2024

This code is already present here:

java/util/src/main/java/io/kubernetes/client/apimachinery/GroupVersion.java

Line 25 in f9c82f3

public static GroupVersion parse(String apiVersion) {

Let's re-use that instead of reimplementing.

brendandburns reviewed

View reviewed changes

util/src/main/java/io/kubernetes/client/util/Yaml.java Outdated

+                    version = apiVersion;
+                  }
+                  //creating resource plurals from kind

Contributor

brendandburns Oct 18, 2024

Consider using the Discovery class here instead

java/util/src/main/java/io/kubernetes/client/Discovery.java

Line 37 in f9c82f3

public class Discovery {

brendandburns reviewed

View reviewed changes

util/src/main/java/io/kubernetes/client/util/Yaml.java Outdated

+                  String resourcePlural = kind.toLowerCase(Locale.ROOT) + "s";  // Simplistic pluralization logic
+                  //getting the strongly typed object from model mapper
+                  Class<?> modelClass = ModelMapper.getApiTypeClass(group, version, kind);

Contributor

brendandburns Oct 18, 2024

I think you can say Class<? implements KubernetesObject> here?

Contributor

brendandburns commented Oct 18, 2024

Some style comments. Generally this code looks good to me.

Please add unit tests.

vaidikcode added 2 commits

October 19, 2024 16:36


          wild imports and parse exceptions

4a0dde2


          addressing other problems

3c7f69c

k8s-ci-robot added cncf-cla: yes and removed cncf-cla: no labels


          adding unit tests

963b83e

Author

vaidikcode commented Oct 21, 2024

@brendandburns I've made the changes to the code and added unit tests, just as you suggested.


          Merge branch 'kubernetes-client:master' into master

f851639

Contributor

brendandburns commented Oct 22, 2024

@vaidikcode looks good to me. I agree with @yue9944882 that this is a better fit for KubectlCreate.java but otherwise ready to merge once you move it there.

Contributor

brendandburns commented Oct 22, 2024

Alternately, feel free to keep the loader code in Yaml.java but move the create code to KubectlCreate

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

Labels

cncf-cla: yes size/L