Closed ctron closed 4 years ago
Sorry for the delay, I am bit short on time at the moment. I will make the changes as soon as possible, as they sound absolutely reasonable.
I've added one commit to https://github.com/Arnavion/k8s-openapi/commits/pr/68 for minor style fixes, and this PR needs to be rebased onto latest master. But first I'm also working out a more general solution for this problem. It seems to me the right way to solve this is to have something like:
trait MapNamespace {
fn map_namespace(&self, path_parts: impl Iterator<&str>) -> impl Iterator<&str>;
}
... where k8s-openapi-codegen
's impl looks like:
fn map_namespace(&self, path_parts: ...) -> ... {
match path_parts {
["io", "k8s", rest @ ..] => std::iter::once("crate").chain(rest.iter().copied()),
path_parts => panic!("unexpected path {:?}", path_parts.join(".")),
}
}
... k8s-openapi-derive
's impl looks like:
fn map_namespace(&self, path_parts: ...) -> ... {
match path_parts {
["io", "k8s", rest @ ..] => std::iter::once("k8s_openapi").chain(rest.iter().copied()),
path_parts => panic!("unexpected path {:?}", path_parts.join(".")),
}
}
... and openshift-openapi-codegen
's impl looks like:
fn map_namespace(&self, path_parts: ...) -> ... {
match path_parts {
["io", "k8s", rest @ ..] => std::iter::once("k8s_openapi").chain(rest.iter().copied()),
["com", "github", "openshift", rest @ ..] => std::iter::once("crate").chain(rest.iter().copied()),
path_parts => panic!("unexpected path {:?}", path_parts.join(".")),
}
}
The usage would be to give it (the components of) a path from the openapi spec and get back (the components of) a path for the Rust types.
This approach would also remove the need for the replace_namespace
function and the replace_namespaces
map that all functions pass around, since they're duplicating the convention of "strip io.k8s prefix, etc" otherwise.
The problem with this approach is for places in the code that use the crate root standalone, like templates that need to create a path to {crate_root}::Resource
. One way is to instead have them generate the full path via map_namespace(&["io", "k8s", "Resource"])
which is equivalent to pretending that there is an io.k8s.Resource
type in the original spec. This seems like a hack, but is already somewhat happening with existing codegen'd types like List
, so I don't think it's a big deal.
I absolutely like your proposal.
The problem with this approach is for places in the code that use the crate root standalone, like templates that need to create a path to
{crate_root}::Resource
. One way is to instead have them generate the full path viamap_namespace(&["io", "k8s", "Resource"])
which is equivalent to pretending that there is anio.k8s.Resource
type in the original spec. This seems like a hack, but is already somewhat happening with existing codegen'd types likeList
, so I don't think it's a big deal.
I saw the same issue. That was the reason add the for_k8s
function. So maybe modify the trait for following helps:
trait MapNamespace {
fn map_namespace(&self, path_parts: impl Iterator<&str>) -> impl Iterator<&str>;
fn map_k8s_namespace(&self, path_parts: impl Iterator<&str>) -> impl Iterator<&str> {
std::iter::once("k8s_openapi").chain(rest.iter().copied())
}
}
impl MapNamespace for K8sOpenApiImpl {
fn map_namespace(&self, path_parts: ...) -> ... {
match path_parts {
["io", "k8s", rest @ ..] => std::iter::once("crate").chain(rest.iter().copied()),
path_parts => panic!("unexpected path {:?}", path_parts.join(".")),
}
}
fn map_k8s_namespace(&self, path_parts: impl Iterator<&str>) -> impl Iterator<&str> {
std::iter::once("crate").chain(rest.iter().copied())
}
}
impl MapNamespace for DerivedImpl {
fn map_namespace(&self, path_parts: ...) -> ... {
match path_parts {
["io", "k8s", rest @ ..] => std::iter::once("k8s_openapi").chain(rest.iter().copied()),
["com", "github", "openshift", rest @ ..] => std::iter::once("crate").chain(rest.iter().copied()),
path_parts => panic!("unexpected path {:?}", path_parts.join(".")),
}
}
}
That way you can call into map_k8s_namespace
for basic types that are in k8s_openapi
and for dynamic types call into map_namespace
. That would be more explicit than relying an a well-known prefix.
k8s-openapi isn't special. openshift-openapi-codegen could have some custom types of its own, and would need a way to detect them and emit them as crate::
, just like k8s-openapi does for its custom types. I think depending on a magic prefix per code generator is fine.
So did you try updating your code generator to use the branch I posted?
So did you try updating your code generator to use the branch I posted?
I just tried it out, works like a charm! Many thanks!
I created a temporary branch here: https://github.com/ctron/openshift-openapi/tree/feature/adopt_k8s_changes_1
Cool. I'll merge it in a few days.
I also need to remove #[doc(hidden)]
and the verbiage around the crate not being stable for third parties.
This is merged, but take note of the change in the k8s-openapi-codegen-common
API in https://github.com/Arnavion/k8s-openapi/commit/872212fd1cfba55e15bfdc7cd9fd0570f7efd5ea , specifically https://github.com/Arnavion/k8s-openapi/commit/872212fd1cfba55e15bfdc7cd9fd0570f7efd5ea#diff-d65b5b9523320f1fff83330daf7f9a80
This is merged, but take note of the change in the
k8s-openapi-codegen-common
API in 872212f , specifically 872212f#diff-d65b5b9523320f1fff83330daf7f9a80
Thanks for your work on this. I just tested the OpenShift generator with your master branch, works perfectly. Pointing out the changes you made, helped quite a lot!
I am looking forward to a release :grinning:
This PR is a base for discussion on the changes I had to do in order to generate OpenShift APIs using the k8s-openapi code generator.
There had been some minor things, like the "tags" being optional, and some OpenShift specific "fix ups". Or the field name generator: OpenShift used query names with dots at some point, or has a field called
ref
andas
, which are Rust keywords.The biggest, and most invasive change was the "crate root" problem. I wanted to re-use, rather then re-generate the
k8s-openapi
create. Still, OpenShift references types from plain K8S, and I wanted them to live in the k8s-open crate. For that it was necessary to generate type names, and make a decision if they are coming from the k8s-openapi crate (which would usek8s-openapi::
as prefix then), or from the openshift-openapi crate (which would usecrate::
as prefix). Depending on the location, a K8S types might get created ask8s-openapi::
orcrate::
.Also, I am pretty sure this can be cleaned up :grinning: