search

Arnavion / k8s-openapi

Rust definitions of the resource types in the Kubernetes client API
Apache License 2.0
373 stars 42 forks source link

also convert '.' to '_' #144

Closed casualjim closed 1 year ago

casualjim commented 1 year ago

Without this I couldn't run cargo test --features v1_27 --features test_v1_27 --all

Arnavion commented 1 year ago

Yes, that was added by 3bfe7c098e6f6686d263e750605d14ef0ce6892e (so I guess OpenShift has some names containing .) and then inadvertently removed by c840bb410cc32b0620b605ed58c392933372ceea. I didn't notice it.

But also it means these tests were not being run in CI, so that needs to be fixed too.

Arnavion commented 1 year ago

Also you didn't rerun the code generator. In fact the generated code is broken now because https://github.com/Arnavion/k8s-openapi/blob/86b90bb446445dbeb792266d0aa6663a05dc5111/k8s-openapi-codegen-common/src/lib.rs#L672 relies on get_rust_ident not rewriting .

Arnavion commented 1 year ago

@ctron Do you happen to remember what names in OpenShift's spec required that get_rust_ident handle names with . in them? I looked at https://github.com/openshift/origin/tree/82d3e63a7e267292a28547b51bc0b79f2c7e2213~1/api/swagger-spec (the commit right before they were deleted from the repo) and I didn't see any such names.

casualjim commented 1 year ago

I see my fix was optimistic. After running codegen it ends up generating invalid code. So that test should just get modified then?

casualjim commented 1 year ago

I made a change that makes the test pass as well as the codegen, but it sounds like it doesn't matter much and the test should just get modified.

ctron commented 1 year ago

@Arnavion thanks for checking. I dug a bit into it, and couldn't out either :grinning: … I also tried to upgrade to the most recent version, and I looks like I fell too much behind. My proposal would be to just ignore it for now. Should I find out, and need it in the future, I would raise a new issue and we could re-add it.

Arnavion commented 1 year ago

Thanks. I'll just delete the test then.