feat: replace unmaintained serde_yaml with maintained serde_yaml_ng

[RoyalOughtness]

See:

• https://github.com/dtolnay/serde-yaml
• https://github.com/acatton/serde-yaml-ng?tab=readme-ov-file#why

[openshift-ci]

Hi @RoyalOughtness. Thanks for your PR.

I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[gemini-code-assist]

Code Review

This pull request correctly identifies the need to replace the unmaintained serde_yaml crate with the maintained serde_yaml_ng fork. This is a good maintenance step. However, the implementation has a syntax error in Cargo.toml that will prevent the project from building. A closing brace is missing in the dependency definition.

[jmarrero]

/ok-to-test

[jmarrero]

Thanks for this, I am curious if this will work without some changes of the code because that cargo.lock did update a bunch of stuff.

[RoyalOughtness]

• fcos-e2e — Job failed.                     BaseSHA:4b099f99d91297eea5e16545fcdb8bcc071661c5

Is this an unrelated issue?

Thanks for this, I am curious if this will work without some changes of the code because that cargo.lock did update a bunch of stuff.

I could see if I can scope down the cargo update if that would be preferred?

[jmarrero]

Yeah if we can keep this PR minimal just replacing serde_yaml with serde_yaml_ng with the less change to the cargo.lock as possible is easier to debug any failures in CI.

[RoyalOughtness]

Yeah if we can keep this PR minimal just replacing serde_yaml with serde_yaml_ng with the less change to the cargo.lock as possible is easier to debug any failures in CI.

@jmarrero Fixed

[cgwalters]

I get the idea of this in principle...but the thing is that what seems to have happened with serde_yaml being deprecated is a 3-way ecosystem split; this is one of top 3 in https://lib.rs/search?q=yaml but honestly most crates just seem to be sticking with serde_yaml...for now.

I guess if I had to weigh in on this what I'd probably say is that what we should actually do is follow Kube and actually deprecate our YAML input and only handle kyaml or so...

[openshift-ci]

@RoyalOughtness: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/fcos-e2e	ed26c22	link	true	/test fcos-e2e

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

[RoyalOughtness]

@cgwalters where are you seeing that most crates are still using serde_yaml? Note that serde_yml is different (and has its own host of issues). Most crates are also using unmaintained packages like paste. I've been going through a number of important packages in the ecosystem and reliance on crates with known vulns and/or deprecations is unfortunately widespread. So I don't think looking at what the majority of crates are doing is a good metric.

Getting rid of serde_yaml altogether makes sense too

[cgwalters]

@cgwalters where are you seeing that most crates are still using serde_yaml?

Well of those, a really big one in the ecosystem is https://lib.rs/crates/kube-client for example.

[cgwalters]

kube-rs/kube#1770 is relevant

[jmarrero]

What about we close this PR in favor of an issue for this repo that mentions kube-rs/kube#1770 so we don't forget to revisit this from time to time?

[kazan417]

I think this is better now yaml_serde
my pull request #5564

[RoyalOughtness]

@cgwalters @jmarrero FYI, they went with serde_saphyr: kube-rs/kube#1975

I can put together a PR to do the same if that works.