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.

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

Chore: Migrate java clients from swagger-codegen to openapi-generator #117

Merged
merged 1 commit into from
Nov 16, 2019
Merged

Chore: Migrate java clients from swagger-codegen to openapi-generator #117

merged 1 commit into from
Nov 16, 2019

Conversation

yue9944882
Copy link
Member

/cc @brendandburns

/hold
(until 5.0.0 released)

note that this pull will involve some critical dependency changes in the java repo.

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 29, 2019
@brendandburns
Copy link
Contributor

@yue9944882 the last time I tried this, it broke a bunch of stuff because the Quantity and other type overrides didn't work correctly. Have you tried compiling this in the Java repo? Does it work?

Before we do this switch, I want to see the PR that it generates for the Java repo....

Thanks

@yue9944882
Copy link
Member Author

yue9944882 commented Jun 10, 2019 •
edited
Loading

@brendandburns i also tried locally but didn't other surprises except for bunch of underlying dependency switching (not sure if it's breaking backward-compatibility). sure i will sent a pull in the repo to reflect the generator changes.

@brendandburns
Copy link
Contributor

To be clear it was the custom mappings like you added in #118 that broke. So if those continue to work correctly, then this LGTM.

@yue9944882 yue9944882 mentioned this pull request Jul 2, 2019
Open
@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 Sep 18, 2019
@davidxia
Copy link
Contributor

/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 Sep 18, 2019
@fabiokung fabiokung mentioned this pull request Sep 21, 2019
Merged
@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 22, 2019
@yue9944882 yue9944882 mentioned this pull request Oct 22, 2019
Closed
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 25, 2019
@yue9944882 yue9944882 force-pushed the chore/migrate-java-client branch 2 times, most recently from 7120c2a to 4c7842c Compare October 28, 2019 06:02
yue9944882
yue9944882 commented Oct 28, 2019
View reviewed changes
openapi/java.xml Outdated
<import-mappings>
IntOrString=io.kubernetes.client.custom.IntOrString,Quantity=io.kubernetes.client.custom.Quantity,V1Patch=io.kubernetes.client.custom.V1Patch
</import-mappings>
<typeMappings>int-or-string=IntOrString,quantity=Quantity</typeMappings>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wing328 for help

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can I help? Can you share more details with me?

@wing328
Copy link

wing328 commented Oct 28, 2019

As discussed over IM, please use the latest stable version v4.1.3 instead as the string's format support together with type mapping is only added in recent versions.

wing328
wing328 reviewed Oct 28, 2019
View reviewed changes
openapi/java.xml
</configOptions>
<typeMappings>int-or-string=IntOrString,quantity=Quantity</typeMappings>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yue9944882 I noticed that you've added https://github.com/kubernetes-client/gen/pull/117/files#diff-4b08c5154861f4aefe8a9bc110c4a15bR155 to map quantity to Quantity so I think we can remove ,quantity=Quatity here (but I've not tested it)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is still necessary. The code you linked is patching the swagger spec to add a format field to the Quantity definition, so we can install a custom mapping here.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

@yue9944882
migrate java clients from swagger-codegen to openapi-generator
bf04565 
add reflection-equality config option

java: update openapi generator to 4.1.2

patch openapi-generator, format Quantity

Bring in a patched openapi-generator temporarily until
openapi-generator#4182 gets merged, and add a format to
resource.Quantity so it can have a custom typeMapping installed.

Also bring configuration of type and import mappings to the correct
(newer) format for the maven plugin.

remove the overrides

refactor

openapi-gen

generator 4.1.3

4.2.0
@brendandburns
Copy link
Contributor

@yue9944882 is this up to date? Now that we've merged the code changes in the java repo, I'd like to merge the corresponding generation code.

Thanks!

@yue9944882
Copy link
Member Author

yes, this is in ready state

@yue9944882
Copy link
Member Author

/hold cancel

@brendandburns for merging

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 13, 2019
@brendandburns
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 16, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brendandburns, yue9944882

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

The pull request process is described here

Needs approval from an approver in each of these files:

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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 16, 2019
@k8s-ci-robot k8s-ci-robot merged commit 270d42b into kubernetes-client:master Nov 16, 2019
@brendandburns brendandburns mentioned this pull request Nov 16, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wing328 wing328 wing328 left review comments

@fabiokung fabiokung fabiokung left review comments

@brendandburns brendandburns Awaiting requested review from brendandburns

Assignees

@brendandburns brendandburns

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@yue9944882 @brendandburns @fejta-bot @davidxia @wing328 @k8s-ci-robot @fabiokung