snowdrop / istio-java-api

A Java API to generate Istio descriptors, inspired by Fabric8's kubernetes-model.
Apache License 2.0
112 stars 33 forks source link

Change builder name to make replace work #12

Closed bgokden closed 6 years ago

bgokden commented 6 years ago

Use io.fabric8.kubernetes.api.builder instead of me.snowdrop.istio.api.builder

Our issue was that when we want to do a replace or update operation on custom istio resource, it was failing with the error that it can not find the constructor. We tried a few solutions and this is the cleanest solution we have.

We can fix the naming of methods if you recommend something else.

metacosm commented 6 years ago

Could you please detail the issue that was happening with the me.snowdrop.istio.api.builder package? That kind of change has a big impact on users of the API so it needs to be well motivated.

avalcepina commented 6 years ago

The problem arises when you want to replace a pre-existing istio resource. You can theoretically achieve that without changing the client, simply by doing something like this:

client .customResources(customResourceDefinition, classOf[IstioResource], classOf[KubernetesResourceList[_ <: HasMetadata]], classOf[DoneableIstioResource]) .inNamespace(client.getNamespace) .createOrReplace(resource)

where resource is the new definition with which you want to replace the pre-existing one (in our case it is a routerule) However this will not work and will instead yield the following exception:

java.lang.NoSuchMethodException: me.snowdrop.istio.api.model.DoneableIstioResource.<init>(me.snowdrop.istio.api.model.IstioResource, io.fabric8.kubernetes.api.builder.Function) at java.lang.Class.getConstructor0(Class.java:3082) at java.lang.Class.getDeclaredConstructor(Class.java:2178) at io.fabric8.kubernetes.client.dsl.base.HasMetadataOperation.replace(HasMetadataOperation.java:115)

The code at the line where this is happening is

D doneable = (D) getDoneableType().getDeclaredConstructor(getType(), Function.class).newInstance(item, visitor); return doneable.done();

And the reason why the error is happening is that the required constructor is not available due to the fact that the DoneableIstioResource class is using me.snowdrop.istio.api.builder.Function instead of io.fabric8.kubernetes.api.builder.Function. Changing the builder solves this problem.

metacosm commented 6 years ago

Gotcha. Thank you, that more or less confirms what I was thinking. I think the issue stems more from the builder generation functionality so I will try to see if it can be addressed there first. What do you think, @iocanel, is there a way to get HasMedataOperation methods working without having builders being forced to be in the io.fabric8.kubernetes.api.builder package?

iocanel commented 6 years ago

Quick answer: You have to reuse the staff in io.fabric8.kuberentes.api.builder that is provided by the kubernetes-model jar (don't need to include them in your jar too).

Longer answer: To support certain aspects of the builders, we need on runtime to reference types as:

...and so on.

That would mean that the generated code would have a run-time dependency on sundrio (the builder generator tool). In many cases that's not desirable, so sundrio provides the option to also generate ALL supporting code inside a configured package, so that builders can use, without bringing in external run-time deps (The kubernetes-model uses that feature and generates all required classes in io.fabric8.kubernetes.api.builder).

Since those builders are already available to your project through the kubernetes-model, I see absolutely no need to generate them again (let alone in an own package). Note: that the kubernetes-client itself does the same by reusing those classes from kubernetes-model:

  @Buildable(builderPackage = "io.fabric8.kubernetes.api.builder", editableEnabled = false)
  public Config() {...}

Which pretty much says reuse the builder package that already exists in io.fabric8.kubernetes.api.builder and don't generate one.

metacosm commented 6 years ago

Thank you!