handling of consecutive upper case letters

[sebhoss]

I'm currently in the progress of re-generating all of https://github.com/metio/kube-custom-resources-rs with the latest and greatest kopium version and noticed that at least one CRD can no longer be generated because of the changes in #365.

The ArgoCDExport kind will be transformed by to_upper_camel_case to ArgoCdExport (notice the lower case 'd') which causes an issue when we call println!(r#"#[kube(status = "{}Status")]"#, kind) latter. We will end up writing #[kube(status = "ArgoCDExportStatus")] but output a struct called ArgoCdExportStatus which causes a compile failure.

I think the original intent of #365 was to upper case the first letter of a kind, and therefore I've adjusted the code here to do just that.

pinging @ClemaX since you wrote the original change & I wanted to say thanks as well for all the great additions lately!

[clux]

thanks for finding this. i think this makes sense, but before i merge, i am a bit confused;

it is a bit unexpected to me that this would create conflicting names to struct which we also generate.
particularly because we also use to_upper_camel_case on all struct names.

wouldn't the struct names we generate align with the naming we struct names we generate when we use the same fn to format?

[sebhoss]

Yeah I think the underlying issue here is that we take the kind straight from the CRD when we do println!(r#"#[kube(status = "{}Status")]"#, kind) in https://github.com/kube-rs/kopium/blob/main/src/main.rs#L227 but otherwise use the name of the structs generated by kopium.

So another way to fix this is to call .to_upper_camel_case on kind in main.rs as well.

[ClemaX]

Ah yes, I didn't think about this edge case and the requirement for a matching status and kind. I think changing only the first letter and leaving the kind as-is is the safest approach. I am not sure if the Kubernetes API is cases-sensitive regarding the kind field.

[clux]

I wonder if it's better to consistently use .to_upper_camel_case everywhere given the names are all consistently formatted by us. Unless you have concerns with how this interacts with the crd catalogue.

[ClemaX]

I would need to test to be sure, but I think resource creation might fail if we use different casing for the kind field.

But we can still use .to_upper_camel_case anywhere (status, structs) but in the kind.

[sebhoss]

I haven't tested it yet but is the latest change what we are talking about here?

My only concern is that this might upset users who currently use the upper cased variant, e.g., ArgoCDExport and now have to modify their code to use ArgoCdExport. However, as long as this is a one-time thing, I fine with doing it like this.

[sebhoss]

welp here we go:

• My original change which just replaced the first character with its upper case variant works for all cases as far as I can see - I'm not sure why though
• Just calling .to_upper_camel_case() on Status while printing does not work since it retains "Spec" (the spec_trimmed_name variable in main.rs) in resources like this (which consists of only upper case letters).
• Calling .to_upper_camel_case() in various other places retains "Spec" in resources originally targeted with Ensure analyze stack initial base name uses upper camel case #365 (resources that start with a lower case letter)

This entire handling of stack-names/container-names/kind-names and the split responsibility of analyzing and printing is really hard to understand. So I would actually go with my original implementation since that seems to work fine and does not break existing users.

In a different project I've recently used https://docs.rs/trycmd/latest/trycmd/ with great success and I think kopium would benefit from that as well. My idea is to run kopium against a set of known CRDs and compare its output with a known good state. Changes like #365 should then show up and maintainers have an easier life reviewing? Should I set up something like this in another PR?

[clux]

Yeah, that makes sense. I can imagine a few CRDs would use an uncommon capitalisation like ArgoCD in the kind name so even if we went for a consistency approach, it's setting us up for a an annoying breaking change.

and afaikt, the consistency approach doesn't even lead to any less warnings, as it's not like we're generating the wrong casing, it's still technically pascalcase. so anyway, yeah, i agree that your first change is probably the less scary one / and simplest one to make here.

Should I set up something like this in another PR?

Please, that's a good idea. We already use trybuild in kube, but the test approach here is less rigorous.

[sebhoss]

OK created #371 for the tests. My plan would be to merge the tests first and rebase this branch on top of that so that we can see the difference in the generated output.

[sebhoss]

OK I think this is it now! Moved .to_upper_camel_case() from analyze one level down to analyze_ and called .to_upper_camel_case() in main.rs every time we reference kind.

The trycmd tests show that we now correctly strip Spec from struct names again and generate Status struct that match what is declared in the kube attribute.

[clux]

i'm a bit confused for the rationale behind trimming Spec. this seems to be a larger breaking change, and i thought you wanted to not break things? 😄

[sebhoss]

😆 I don't want to introduce a breaking change but my understanding is that this already happened in #365 for CRDs which use a kind value that is not already in upper camel case form. When you look at the trycmd tests, you can see that everything works fine in the current main branch for CRDs that use an upper camel case form: https://github.com/kube-rs/kopium/blob/main/tests/cmd/generate/from-file.md?plain=1#L218-L305. For these, Spec gets trimmed from struct names and the Status struct name matches. For CRDs that are not in upper camel case form, this does not work currently: https://github.com/kube-rs/kopium/blob/main/tests/cmd/generate/from-file.md?plain=1#L104-L216. Here we can see that generated structs retain Spec in their names and the Status struct name does not match (ArgoCDExportStatus vs ArgoCdExportStatus).

So this PR introduces a breaking change when compared to current main, but does not introduce a breaking change when compared to main before #365 - it reverts a breaking change? 😅

[clux]

i had a test of my local repos this morning and it does indeed look safer than i thought.

i thought the spec pruning that i see in the diff here now would affect everything, but it seems all my repos are untouched by it.

still, trying to understand why Spec can safely go away when that was not part of #365
EDIT: understand now

[clux]

So this PR introduces a breaking change when compared to current main, but does not introduce a breaking change when compared to main before #365 - it reverts a breaking change? 😅

This still looks breaking, and unrelated to #365 though?

[clux]

When you look at the trycmd tests, you can see that everything works fine in the current main branch for CRDs that use an upper camel case form: https://github.com/kube-rs/kopium/blob/main/tests/cmd/generate/from-file.md?plain=1#L218-L305. For these, Spec gets trimmed from struct names and the Status struct name matches. For CRDs that are not in upper camel case form, this does not work currently: https://github.com/kube-rs/kopium/blob/main/tests/cmd/generate/from-file.md?plain=1#L104-L216.

oh, wait, we used to trim Spec before, but it didn't always work. ok i think i get it now.

[clux]

merged, but parts of me is questioning my own logic on trimming just the Spec part in general from structs leaving all the other prefix intact. technically we could have taken out the whole Kind prefix also on all child structs.

not sure that's something worth doing at this point though.

[sebhoss]

I think keeping the Kind prefix reduces the potential for name conflicts? Spec does not really add anything 🤷

[clux]

thanks again for being so thorough (and remining me of my own questionable code)

happy to do a release at this point unless you have anything else in the pipeline!

[sebhoss]

Another release would be great! I'm currently going through all .ignore files I have in https://github.com/metio/kube-custom-resources-rs and review whether they are still required. So far, it seems all the great changes recently have allowed me to remove them entirely which means that kopium is able to generate compilable Rust code for all CRDs 🎉

Kinda related to that is CustomResourceDefinition/catalog#74 but that should not matter for new releases of kopium

[clux]

here you go; https://github.com/kube-rs/kopium/releases/tag/0.22.4

kopium is able to generate compilable Rust code for all CRDs 🎉

i guess it's time to remove the "not feature complete" warning in the readme at this point 😄

[sebhoss]

here you go; https://github.com/kube-rs/kopium/releases/tag/0.22.4

kopium is able to generate compilable Rust code for all CRDs 🎉

i guess it's time to remove the "not feature complete" warning in the readme at this point 😄

Thanks! The last run on friday already picked this up. There are 31 cases remaining which cannot be handled by kopium at the moment. I think most of those are related to #66 and 3 of those could be fixed with #373

[clux]

Ah, thanks. I'll correct the README before releasing.

Interesting that it's mostly naming... I hadn't thought much about the solution to it because I hadn't seen it myself.
But insert time deduplication could be done to get default kopium to work, but maybe there's value in the the config approach in #327 as a prettier approach for a catalogue library. Probably need to help resurrect that.