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

openapi gen standardisation pre-donation #606

Closed
clux opened this issue Aug 4, 2021 · 5 comments
Closed

openapi gen standardisation pre-donation #606

clux opened this issue Aug 4, 2021 · 5 comments
Labels
cncf cncf requirements or donation related invalid rejected as a valid issue question Direction unclear; possibly a bug, possibly could be improved.

Comments

@clux
Copy link
Member

clux commented Aug 4, 2021

Continuation of comments from the root issue around gen, k8s-openapi, kubernetes' desire to centralise as much of the code gen as possible.

This meant to be the key points. Please comment if I am missing some details or am getting something wrong here. Trying to use this to link to in the greater discussion.

Highlighted comments:

Points

Proto

We could potentially look at starting in gen for proto in #371 (if arnavion's right it might render the patchup stage less needed).

  • gen's generation script is clean
  • however: protoc does not support rust out of the box so we'd have to just bail out after downloading (and maybe patching), and then rely on prost (leaving the gen repo, yet again, as an awkward layer of indirection)
  • this probably leaves a pretty big piece of work to get the generics out of it, no idea how much of k8s-openapi can be re-used
  • on the plus side, we know what the good outcome looks like, and can compare with k8s-openapi output

Options

  • native openapi generator looks completely infeasible
  • jumping through gen for openapi and bailing early: does not feel great for us, and might not feel great for them either (esp. if we avoid 95% of their code)
  • jumping through gen for a proto exploration in protobuf encoding exploration #371 is something we wanted to try anyway (might be beneficial for both parties)
  • creating a cli tool in gen to download and patch the swagger might be an alternative way to avoid the awkward repo dependency step
  • codifying a cleaner system for upstreaming (non-rust specific) fixups is ultimately beneficial to all (and probably easy for us)
@clux clux mentioned this issue Aug 4, 2021
7 tasks
@clux clux added cncf cncf requirements or donation related question Direction unclear; possibly a bug, possibly could be improved. labels Aug 4, 2021
@kazk
Copy link
Member

kazk commented Aug 5, 2021

kube probably doesn't fit well in their process because it's not a basic client they have in mind. They justify creating an official client even when there are good clients with this:

There are good clients for each of these languages, but having a basic supported client would even help those client libraries to focus on their interface and delegate transport and config layer to this basic client.

Languages - Kubernetes: New Client Library Procedure

(This doesn't apply to us at all because a naively generated official client won't be helpful. I also don't know if official clients in other languages are used like that.)

What they have in mind is more towards k8s-openapi with api feature enabled than kube. I tried to explain this in kubernetes/org#2792 (comment).


I haven't looked into gen much yet, but they mentioned C# client is using a different generator, and it looks like it uses autorest. https://github.com/kubernetes-client/gen/blob/636729c92ea425017872235e9d5758b9dd8ce85f/openapi/csharp.xml#L35

It'll be awkward (and pointless) to call k8s-openapi-codgen like that, though.

Can add an option to generate from a file instead, and read the patched file. But again, it's awkward and pointless. Also, src/lib.rs is not generated.


reasonable to want to move DL + patching of the spec out from their POV, but not sure how easy (or how much of a point) that would be

creating a cli tool in gen to download and patch the swagger might be an alternative way to avoid the awkward repo dependency step

There's only one fix up they benefit from at the moment because they don't backport fixes, and everything else is fixed in 1.22. For that one, downloading and patching swagger.json is easy with curl + gron + perl/sed:

curl -sSL $SWAGGER_URL \
| gron \
| perl -pe 's/(?<=kind = ")(Pod|Node|Service)(?:Attach|Exec|PortForward|Proxy)Options(?=")/$1/' \
| gron -u \
> swagger.json 

@kazk
Copy link
Member

kazk commented Aug 5, 2021

See #371 (comment) for the default prost code gen output.

@clux
Copy link
Member Author

clux commented Aug 5, 2021

kube probably doesn't fit well in their process because it's not a basic client they have in mind. They justify creating an official client even when there are good clients with this:

There are good clients for each of these languages, but having a basic supported client would even help those client libraries to focus on their interface and delegate transport and config layer to this basic client.

I do think they kind of illustrate how difficult it is to use a basic client inside an advanced one by the existence of kubernetes-client/go vs. kubernetes/client-go. It's hard to slot these things together, and this goes doubly in rust where the ecosystem has moved so fast with dependencies and the ways best practice clients have been going.

Anyway, their justification aside, some of the dedicated clients DO manage to do a bit of generic stuff on top of the generation.
In particular the C# client has these templates

So it's possible that the autorest setup is a lot more powerful. Although it does not look like it supports rust.

@clux
Copy link
Member Author

clux commented Aug 5, 2021

Still, it'd be a CI nightmare to have kube (the crate) inside kubernetes-client and the rest of kube-rs elsewhere, so I still don't think we fit inside the kubernetes-client org. I'll make a comment on the main thread.

@clux
Copy link
Member Author

clux commented Oct 30, 2021

Closing this for now as it's no longer a requirement for donation (and that's why it was openend). That said, we are working on the protobuf issue #371 in the https://github.com/kube-rs/k8s-pb repo.

@clux clux closed this as completed Oct 30, 2021
Repository owner moved this from Defining to Done in Kube Roadmap Oct 30, 2021
@clux clux removed this from Kube Roadmap Oct 30, 2021
@clux clux changed the title openapi gen standardisation openapi gen standardisation pre-donation Oct 30, 2021
@clux clux added the invalid rejected as a valid issue label Oct 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf cncf requirements or donation related invalid rejected as a valid issue question Direction unclear; possibly a bug, possibly could be improved.
Projects
None yet
Development

No branches or pull requests

2 participants