Code needs regenerated to use Quantity object

lewisheadden commented 6 years ago

I was looking at the V1ResourceRequirements object and noticed these lines

  @SerializedName("limits")
  private Map<String, String> limits = null;

  @SerializedName("requests")
  private Map<String, String> requests = null;

but I think they should be (and when I run update-client.sh they are)

  @SerializedName("limits")
  private Map<String, Quantity> limits = null;

  @SerializedName("requests")
  private Map<String, Quantity> requests = null;

Is there any reason when we regenerated to fix byte[] issues we didn't bring this change in too?

cc/ @brendandburns

brendandburns commented 6 years ago

I probably messed something up... I will try to regenerate.

brendandburns commented 6 years ago

Actually, I looked into this a little more, this is modelled in swagger as additionalProperties

  "io.k8s.api.core.v1.ResourceRequirements": {
    "description": "ResourceRequirements describes the compute resource requirements.",
    "properties": {
     "limits": {
      "description": "Limits describes the maximum amount of compute resources allowed. More info: https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/",
      "type": "object",
      "additionalProperties": {
       "$ref": "#/definitions/io.k8s.apimachinery.pkg.api.resource.Quantity"
      }
     },
     "requests": {
      "description": "Requests describes the minimum amount of compute resources required. If Requests is omitted for a container, it defaults to Limits if that is explicitly specified, otherwise to an implementation-defined value. More info: https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/",
      "type": "object",
      "additionalProperties": {
       "$ref": "#/definitions/io.k8s.apimachinery.pkg.api.resource.Quantity"
      }
     }
    }
   },

And it looks like swagger-codegen and Java doesn't respect additionalProperties

https://github.com/swagger-api/swagger-codegen/issues/5187

@mbohlool @lwander fyi

lewisheadden commented 6 years ago

@brendandburns I thought the process of preprocessing the spec in preprocess_spec.py was meant to address this. Let me take a look when I'm back in the office on Wednesday and get back to you.

brendandburns commented 6 years ago

I am definitely not an expert here, but I think the problem is that those types never even make it into the Java object model to be replaced.

As far as I can tell, the 'additionalProperties' field in the swagger spec is ignored by the Java code generator.

From: Lewis Headden notifications@github.com Sent: Sunday, March 18, 2018 2:51 PM To: kubernetes-client/java Cc: Brendan Burns; Mention Subject: Re: [kubernetes-client/java] Code needs regenerated to use Quantity object (#209)

@brendandburnshttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fbrendandburns&data=04%7C01%7Cbburns%40microsoft.com%7C2ffec482daf4412e215d08d58d1a7012%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636570067047119483%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwifQ%3D%3D%7C-1&sdata=6Mipb4%2BSGOcWmqo9USIXlCQgMPK1Hz8g3p2qL8OMAEw%3D&reserved=0 I thought this change to the Maven pomhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fkubernetes-client%2Fgen%2Fblob%2Fmaster%2Fopenapi%2Fjava.xml%23L48-L49&data=04%7C01%7Cbburns%40microsoft.com%7C2ffec482daf4412e215d08d58d1a7012%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636570067047119483%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwifQ%3D%3D%7C-1&sdata=FympqKPIQj2gnTsltBU2crTvjQqlw7N%2FEaqQybwpaJc%3D&reserved=0 changed that behavior?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fkubernetes-client%2Fjava%2Fissues%2F209%23issuecomment-374053047&data=04%7C01%7Cbburns%40microsoft.com%7C2ffec482daf4412e215d08d58d1a7012%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636570067047119483%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwifQ%3D%3D%7C-1&sdata=wQ3K9OW0AkrqtZ8vHF3nuBSlpSAEr7Kpanap3r0n47w%3D&reserved=0, or mute the threadhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAFfDgpsTBGQ3vJUJsw8WKEUL_NLd1havks5tftbtgaJpZM4SpgC9&data=04%7C01%7Cbburns%40microsoft.com%7C2ffec482daf4412e215d08d58d1a7012%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636570067047119483%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwifQ%3D%3D%7C-1&sdata=E2tHUbYepHpDUC3IGSxQ0qq7j2jyXLGD2MmDSYp1WLo%3D&reserved=0.

lewisheadden commented 6 years ago

I did a fresh git clone of this repo and ran ./scripts/update-client.sh and receive a definition of Map<String, Quantity>.

brendandburns commented 6 years ago

I figured this out... There are actually 2 branches in the swagger-codegen project master (2.4.0) and 3.0.0

The current gen is using 3.0.0 but the commit for the serialization fix is only in the master branch.

It looks like only the 3.0.0 branch supports the additionalProperties stuff.

I'm going to hack up the gen project to enable cherry-picks (and hopefully the pick will apply)

brendandburns commented 6 years ago

This is fixed with #218

kubernetes-client / java

Code needs regenerated to use Quantity object #209