Skip to content

Conversation

mbohlool
Copy link
Contributor

I've followed other client's pattern to add a setting file and generate the haskell client in a folder named kubernetes/. cc @guoshimin @brendandburns

@guoshimin
Copy link
Contributor

@jonschoning This was generated using your HaskellHttpClientCodegen. Would love your input!

@jonschoning
Copy link
Contributor

jonschoning commented Dec 22, 2017

Awesome!

Do you plan to re-gen the code if the kubernetes swagger spec changes, or is this a one-time only generation?

This is a pretty big spec; My generator puts all the operations everything into one file, which may or may not work for you. Another option would be to split the kubernetes swagger spec into smaller swagger spec

I should point out HaskellHttpClientCodegen is intends to follow OAS2.0 as closely as possible. Many vendors have extensions which aren't supported.

Because OAS2.0 doesn't allow nulls, (nulls are stripped from JSON requests) some of the JSON mime roundtrip tests fail when the data model includes an arbitrary hashmap/bag of values/A.Value. (I think the tests fail when rendering an (Just empty hashmap), because it gets serialized back as a Nothing. So whether kubernetes needs to differentiate between empty bags of values, vs a value not present, would be something to watch out for. (there also seems to be a certain model in the code generated here which causes quickcheck to hang)

the logging was made to be as brain-dead as possible for someone using a client, but for more advanced cases of logging, or for more robust logging to files, i think the Client.hs would have to be adjusted. by default it uses Katip for logging, but MonadLogger is also an option.

since you have some custom mime types, the generator doesn't know how to render those, so those definitions would have to be filled out; if they are rendered the same as json, then you can copy the MimeJson code for those other mimetypes

Also it seems that a bunch of operations have a mimetype of */* or MimeAny. This means the Haskell code won't know how to render that data out of the box. perhaps it could also use MimeJson logic for MimeAny, but I don't make that assumption.

@brendandburns
Copy link
Contributor

brendandburns commented Dec 22, 2017 via email

@jonschoning
Copy link
Contributor

jonschoning commented Dec 23, 2017

One idea would be to just re-gen Model.hs and API.hs and ModelLens.hs , which are the only dynamic parts. Then you could customize the client and core logic as you see fit outside of generation. API.hs really only creates a data structure which describes the request (KubernetesRequest). So you should have flexibility outside of generation to interpret the data structure with whatever logic you want, using the existing logic as a starting point

I am curious with how the other language generators deal with non-json content types and accept header options

@guoshimin
Copy link
Contributor

Thanks @jonschoning. These are very helpful.

Curious why would (Just empty hashmap) roundtrip to Nothing? Wouldn't it get serialized to {} and then back to (Just empty hashmap)?

Regarding MimeAny, I see that in the Kubernetes OpenAPI spec, all the POST/PUT methods are marked as consuming */*, which, when translated into Haskell code, becomes a requirement to specify MimeAny as Content-Type. This seems backwards to me. I'm not an expert on OpenAPI, but shouldn't it be interpreted as any content-type is OK? I mean, the client must tell the server how the body was encoded, not just */*, right? As it stands now, these methods don't work.

For the other custom MIME types, I see there are three categories:

  • patch-related: The methods that use these MIME types already have A.Value as the body type and the MIME types can render JSON, so we are OK there.
  • protobuf: I think it's OK that we don't support it for now
  • yaml: I think it's OK that we don't support it for now

I looked at the python client. It doesn't seem to support protobuf and yaml either.

@guoshimin
Copy link
Contributor

Also, splitting would be nice. Sometimes GHC OOMs on Model.hs when I have too many things running on my box. I'm not sure how one goes about splitting an OpenAPI spec though.

@jonschoning
Copy link
Contributor

jonschoning commented Dec 23, 2017

Curious why would (Just empty hashmap) roundtrip to Nothing? Wouldn't it get serialized to {} and then back to (Just empty hashmap)?

I just reviewed this in some more detail. The (Just empty hashmap) case indeed roundtrips fine. It is actually an issue with Aeson's Null constructor (I forgot that Aeson's A.Value type which represents JSON includes a constructor for A.Null)

If swagger model is a Maybe json object, Maybe A.Value:

  1. quickcheck will generate values of Just A.Null.
  2. nulls will be stripped from the serialized output
  3. missing/null values from the json output will be parsed into Nothing which doesn't match Just A.Null, hence the failing test

I'm not sure if this is an actual issue for you or not to have to distinguish between Just A.Null and Nothing


I should note that a swagger type of "object" is always mapped to aesons's Value type, so for this model, the maxSurge and maxUnavailable will be of type Maybe A.Value (Maybe because they are not required)

    "v1beta2.RollingUpdateDeployment": {
      "description": "Spec to control the desired behavior of rolling update.",
      "properties": {
        "maxSurge": {
          "type": "object",
          "format": "int-or-string"
        },
        "maxUnavailable": {
          "type": "object",
          "format": "int-or-string"
        }
      }
    },

Regarding MimeAny, I see that in the Kubernetes OpenAPI spec, all the POST/PUT methods are marked as consuming */*, which, when translated into Haskell code, becomes a requirement to specify MimeAny as Content-Type.

This is the first time I've encountered extensive use of *\* in an api, so it's likely I've misunderstood what *\* meant, and generator could be improved for this situation.

@guoshimin
Copy link
Contributor

@mbohlool Do you know why a lot of the operations in the OpenAPI spec say they consume */*? And what does it mean exactly?

@jonschoning
Copy link
Contributor

@brendandburns
Copy link
Contributor

@brendandburns
Copy link
Contributor

@jonschoning is there a way to tell the generator only to generate those files? Does it obey the standard .swagger-ignore?

@brendandburns
Copy link
Contributor

(this LGTM, btw, we can merge and iterate)

@brendandburns
Copy link
Contributor

@mbohlool can I have write access to this repo?

@guoshimin
Copy link
Contributor

@jonschoning I tried a POST request with your latest patch and it works. Thanks!

@guoshimin
Copy link
Contributor

As a side note, swagger-codegen now doesn't compile under jdk7, which is what the kubernetes-client/gen repo uses. I will send a PR to upgrade the jdk version.

@jonschoning
Copy link
Contributor

jonschoning commented Dec 27, 2017

Yes it should obey .swagger-ignore.

Yet another way to iterate considering spec updates would be to "wrap" the generated library with another library, consuming the generated output as a dependent library and re-exporting it's model/api's. This, or the "single-library/.swagger-ignore" technique would both work depending on your preference.

One thing I'm noticing that bugs me, relevant to this, is that there are a significant number of operations that have the same operationId under different namespaces/tags. The other generators separate out tags into different namespaces, so there aren't any naming conflicts. But as the haskell-http-client generator currently puts all operations into one module, to avoid naming conflicts it must generate a unique name via a number causing 19 versions of getAPIGroup, which technically does work / compile / execute, but seems a bit error prone to actually program with. https://gist.githubusercontent.com/jonschoning/52ff3dbc95078a2a5d10486bd16139af/raw/1f743b1e0e2e8f963fac67782eeccfd95f9eb381/getAPIGroupX.txt

This behavior of haskell-http-client generator to put ops and models in one module was a design decision to simplify the generated output for various specs I tested with, but it seems like the wrong default for a spec like kubernetes with a high number of similarly-named operations and models in different tags/namespaces.

I'm not sure yet how much effort it would take to get the haskell-http-client generator to create a separate module per tag/namespace, but something to consider if it ends up changing in the future how to incorporate that into how to version/iterate, as changing that would create more files per module/tag/namespace

@jonschoning
Copy link
Contributor

jonschoning commented Dec 28, 2017

It appears creating a module per tag/namespace for each "api", to solve the above issue is very doable.

I have a proof-of-concept already working. After finishing it i'll send them another PR.

@guoshimin
Copy link
Contributor

@jonschoning Awesome!

@brendandburns Are we just waiting for someone with write access to merge this?

@jonschoning
Copy link
Contributor

jonschoning commented Dec 28, 2017

I've added commit swagger-api/swagger-codegen@fd760ef to swagger-api/swagger-codegen#7254 which generates separate api modules, for each child

With this in place, I think you guys should be in a good position to iterate, good luck!

@mbohlool mbohlool merged commit b74dede into kubernetes-client:master Dec 28, 2017
@mbohlool
Copy link
Contributor Author

I've merged this PR based on @brendandburns 's LGTM. I will give write access so you guys can iterate.

About changing static part of the generated client, it is good idea to iterate fast and we did that in python client and add them to swagger codegen ignore file but we revert those changes and add them in swagger codegen to get newer version of generator (which may have change those static parts).

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

Successfully merging this pull request may close these issues.

4 participants