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

migrate from swagger-codegen to openapi-generator #93

Closed
tomplus opened this issue Dec 15, 2018 · 19 comments · Fixed by #138
Closed

migrate from swagger-codegen to openapi-generator #93

tomplus opened this issue Dec 15, 2018 · 19 comments · Fixed by #138

Comments

@tomplus
Copy link
Member

tomplus commented Dec 15, 2018

This repo uses the swagger-codegen, which is not actively maintained, at least from a my point of view. It makes lots of hacks like post-generation patching and it’s very hard to maintain these scripts.

For instance - here is a list of PRs which are ready to merge but are not merged yet and had to be applied as patches in Python generator:

A lot of contributors moved from the swagger-codegen to the new project - openapi-generator. Basically it’s a fork of swagger-codegen but it’s actively developed and it gains popularity very fast.

All above PRs are already ported and merged to the openapi-generator:

What do you think about changing generator? Is it possible? I’ll be happy to work on it...

@brendandburns
Copy link
Contributor

Yes we should do this. If you have cycles for the PR that's awesome. Otherwise I'll try to get to it.

@mbohlool
Copy link
Contributor

@mbohlool
Copy link
Contributor

@tomplus Thanks for doing this. Please make sure at least supported client such as python or java would create identical client before switching to openapi-gen. There are documents (e.g. this) that may need to be updated to access the right generator.

@tomplus
Copy link
Member Author

tomplus commented Jan 22, 2019

@mbohlool Thanks for your advice. I'll check these clients and update docs too.

Some changes in generated code will appear because clients are based on very old version of generator - java client:

SWAGGER_CODEGEN_COMMIT="${SWAGGER_CODEGEN_COMMIT:-5d263e1c9cdd395d93adf061c63d5ef58a8e9ec5}"; \
committed on Aug 9, 2017

python:

SWAGGER_CODEGEN_COMMIT="${SWAGGER_CODEGEN_COMMIT:-d2b91073e1fc499fea67141ff4c17740d25f8e83}"; \
committed on Sep 26, 2017

I hope we'll get working clients with a small set of non-breaking changes.

@brendandburns
Copy link
Contributor

I tried the Java generator. It seems to fail on the type-mappings directive. I'm working with the maintainers of openapi-generator to see if there's a fix.

@tomplus, I will own this for Java and Typescript.

Thanks!

@tomplus
Copy link
Member Author

tomplus commented Jan 26, 2019

@brendandburns Thanks 👍 I'm working on the Python client.

@wing328
Copy link

wing328 commented Jan 26, 2019

UPDATE: I did some tests and figure out why the type-mapping behaves differently:

To start with, I think openapi-generator is doing the right thing (we've improved the logic to resolve aliases to primitive type). Here is the definition of intstr.IntOrString in the k8s spec:

    "intstr.IntOrString": {
      "description": "IntOrString is a type that can hold an int32 or a string.  When used in JSON or YAML marshalling and unmarshalling, it produces or consumes the inner type.  This allows you to have, for example, a JSON field that can accept a name or number.",
      "type": "string",
      "format": "int-or-string"
    },

which is not a model but a string (primitive type) with a special format (int-or-string) so the following model (as an example) is resolving the property type as string correctly:

    "v1beta2.RollingUpdateDeployment": {
      "description": "Spec to control the desired behavior of rolling update.",
      "properties": {
        "maxSurge": {
          "description": "The maximum number of pods that can be scheduled above the desired number of pods. Value can be an absolute number (ex: 5) or a percentage of desired pods (ex: 10%). This can not be 0 if MaxUnavailable is 0. Absolute number is calculated from percentage by rounding up. Defaults to 25%. Example: when this is set to 30%, the new ReplicaSet can be scaled up immediately when the rolling update starts, such that the total number of old and new pods do not exceed 130% of desired pods. Once old pods have been killed, new ReplicaSet can be scaled up further, ensuring that total number of pods running at any time during the update is atmost 130% of desired pods.",
          "$ref": "#/definitions/intstr.IntOrString"
        },
        "maxUnavailable": {
          "description": "The maximum number of pods that can be unavailable during the update. Value can be an absolute number (ex: 5) or a percentage of desired pods (ex: 10%). Absolute number is calculated from percentage by rounding down. This can not be 0 if MaxSurge is 0. Defaults to 25%. Example: when this is set to 30%, the old ReplicaSet can be scaled down to 70% of desired pods immediately when the rolling update starts. Once new pods are ready, old ReplicaSet can be scaled down further, followed by scaling up the new ReplicaSet, ensuring that the total number of pods available at all times during the update is at least 70% of desired pods.",
          "$ref": "#/definitions/intstr.IntOrString"
        }
      }
    },

To make the k8s spec works with type-mapping in openapi-generator, here is a way to do it:

  • remove "intstr.IntOrString" (the following) from the spec and you will get the desired result:
    "intstr.IntOrString": {
      "description": "IntOrString is a type that can hold an int32 or a string.  When used in JSON or YAML marshalling and unmarshalling, it produces or consumes the inner type.  This allows you to have, for example, a JSON field that can accept a name or number.",
      "type": "string",
      "format": "int-or-string"
    },

(another way is to convert the JSON file into YAML and comment out the above (alias) definition.

@ackintosh
Copy link
Contributor

I started working on the Ruby client. 💨

@ackintosh
Copy link
Contributor

memo: the haskell client has been switched. #108

@ackintosh
Copy link
Contributor

UPDATE: the Ruby client generator has been switched to openapi-generator. #103

@007
Copy link

007 commented Apr 30, 2019

@tomplus any update on the Python client?

I'm waiting on swagger-api/swagger-codegen#8061 to get kubernetes-client/python#411 resolved. It seems you made some progress on that, I haven't had a chance to update and test my broken code to see if the workaround fixes the hang.

@tomplus
Copy link
Member Author

tomplus commented Apr 30, 2019

@007 The PR (#97) is ready to review, I hope it will be merged soon.

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 29, 2019
@tomplus
Copy link
Member Author

tomplus commented Jul 30, 2019

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 30, 2019
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 28, 2019
@tomplus
Copy link
Member Author

tomplus commented Nov 8, 2019

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 8, 2019
@brendandburns
Copy link
Contributor

brendandburns commented Nov 16, 2019

@tomplus we just switched the Java generator in #117. At this point I think everyone is on openapi?

Can we close this issue?

@tomplus
Copy link
Member Author

tomplus commented Nov 16, 2019

@brendandburns Wow! Great news. I did some checks and it looks like csharp client still uses the old generator. Javascript client also but it likely doesn't use this repo to generate (I suggest to remove its script to avoid confusion). Python-asyncio needs some cleanups, one unused variant uses swagger - I'll remove it.

Should we remove swagger-codegen directories and files when all generators has been switched or maybe you want to leave it for a while?

@brendandburns
Copy link
Contributor

csharp uses autorest, which is a totally different generator.

Javascript can be removed as we're using typescript.

I'll send PRs to do the cleanup.

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 a pull request may close this issue.

8 participants