Skip to content

Update API reference generator for 1.28 #325

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

Merged
merged 1 commit into from
Aug 11, 2023

Conversation

tengqm
Copy link
Contributor

@tengqm tengqm commented Aug 11, 2023

No description provided.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 11, 2023
@k8s-ci-robot k8s-ci-robot requested a review from kbhawkey August 11, 2023 11:57
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tengqm

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 requested a review from sftim August 11, 2023 11:57
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 11, 2023
@sftim
Copy link

sftim commented Aug 11, 2023

Could you explain the change to initOperationParameters() @tengqm? It looks reasonable but I don't have context for why we're updating that function here.

@tengqm
Copy link
Contributor Author

tengqm commented Aug 11, 2023

Could you explain the change to initOperationParameters() @tengqm? It looks reasonable but I don't have context for why we're updating that function here.

The new swagger doc starting v1.28 has some drastical changes to how path parameters are documented. Previously, the swagger generator (in the upstream source code) generates the complete list of parameters for each and every operation, and the parameters are repeatedly documented underneath that operation. For example, every "listNamespacedFooBar" operation has a watch parameter documented. This means the how swagger spec is large, with duplicated contents.

Starting with v1.28, the generator was "optimized". All parameters are extracted to the top level swagger parameters block, leaving the per-operation parameter definition merely a reference to the global one. The has heavily reduced the spec size. It was a good intent.

Our previous code wasn't able to handle this case. So we have to parse the parameter reference, fetch the global parameter definitions and document them on a per-operation basis.

There are significant bugs in the generated swagger today. Mainly because the logic cannot handle duplicated parameter names properly. For example, for connectNamespacedPod....ProxyWithPath operations, there are two parameters named path, one in the URL path, the other in the body. The generated swagger in 1.28 is not handling this correctly. That is something to be fixed in the upstream.

@sftim
Copy link

sftim commented Aug 11, 2023

There are significant bugs in the generated swagger today. Mainly because the logic cannot handle duplicated parameter names properly. For example, for connectNamespacedPod....ProxyWithPath operations, there are two parameters named path, one in the URL path, the other in the body. The generated swagger in 1.28 is not handling this correctly. That is something to be fixed in the upstream.

Is there an issue? Let's try to file one.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 11, 2023
@k8s-ci-robot k8s-ci-robot merged commit ea728e4 into kubernetes-sigs:master Aug 11, 2023
@tengqm tengqm deleted the apiref-128 branch August 11, 2023 13:07
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.

3 participants