Skip to content

Regenerate with new openapi generator #850

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

Conversation

uesyn
Copy link

@uesyn uesyn commented Jul 28, 2022

This PR is for release-1.x branch.

openapi-generator supported import-mapping for typescript client generator.
ref: OpenAPITools/openapi-generator#12957

With it, we don't need to do workaround like copying IntOrString type to IntOrString.ts.
In this PR, re-generate codes using openapi generator which supports import-mapping.
In addition, cherry-pick #830.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 28, 2022
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 28, 2022
@uesyn
Copy link
Author

uesyn commented Jul 28, 2022

/cc @davidgamero

@k8s-ci-robot
Copy link
Contributor

@uesyn: GitHub didn't allow me to request PR reviews from the following users: davidgamero.

Note that only kubernetes-client members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @davidgamero

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@brendandburns
Copy link
Contributor

cc @davidgamero

looks like a number of the paths changed here? (e.g. the models directory was added) do we know why things moved around? It doesn't necessarily bother me, but I want to make sure that it is known/intentional.

@brendandburns
Copy link
Contributor

Also, we really aren't running any unit tests on this branch so we need to be extra careful.

@davidgamero
Copy link
Contributor

hello @uesyn, nice to see you again!
the input mapping support would make this transition much easier compared to the workarounds I've been using.
It seems worth it to me to figure out the changes and jump to this commit for the gen.

@davidgamero
Copy link
Contributor

let me take a shot at getting a couple more unit tests running this weekend so we can verify we can retain progress.
It'd be nice to at least keep our current progress on unit tests when making this kind of generated file change

@uesyn
Copy link
Author

uesyn commented Jul 29, 2022

@brendandburns
cc: @davidgamero

looks like a number of the paths changed here? (e.g. the models directory was added) do we know why things moved around? It doesn't necessarily bother me, but I want to make sure that it is known/intentional.

When import mapping feature is added to openapi-generator, the template of generated codes was changed to simplify openapi-generator implementation.
As a result, generated import path become redundant. This is the why that models is added in some lines.
for example import { V1CronJobSpec } from './V1CronJobSpec'; was converted to import { V1CronJobSpec } from '../models/V1CronJobSpec';

ref: OpenAPITools/openapi-generator#12957 (comment)

The reason why that the lines related to IntOrString and V1MicroTime is changed is the result of generating with import mapping feature. (main purpose of this PR)

The other lines changed is the result that supportES6 option was reflected.
ref: https://github.com/kubernetes-client/gen/blob/master/openapi/typescript.xml#L31

@davidgamero
Copy link
Contributor

i tried running the tests on your branch and it looks like you forked at #44f5170046d7cecd0738935942ce2d92f7422690 which is a couple commits behind the current state of the release-1.x branch.

Could you merge the release-1.x branch into yours or regenerate on top of the current head so I can give it a try?
@uesyn

@uesyn uesyn force-pushed the regenerate-with-new-openapi-generator branch from 69d73ae to cf0714c Compare August 3, 2022 13:32
@uesyn
Copy link
Author

uesyn commented Aug 3, 2022

@davidgamero
Completed to rebase and regenerate.
Could you try it, please?

@davidgamero
Copy link
Contributor

yep i can confirm that the unit tests run and the progress is retained
/lgtm
@uesyn @brendandburns

@k8s-ci-robot
Copy link
Contributor

@davidgamero: changing LGTM is restricted to collaborators

In response to this:

yep i can confirm that the unit tests run and the progress is retained
/lgtm
@uesyn @brendandburns

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@brendandburns
Copy link
Contributor

/lgtm
/appprove

Thanks! (apologies for the delay, I was on vacation)

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 8, 2022
@brendandburns
Copy link
Contributor

/approve

(instead of /appprove :P)

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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 Aug 11, 2022
@k8s-ci-robot k8s-ci-robot merged commit ca44c13 into kubernetes-client:release-1.x Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants