refactor: replace deprecated serde_yaml with serde_saphyr#2492
Merged
Conversation
49db8ec to
c1b6c13
Compare
Contributor
|
From my side we could merge it. Should we mention that on a Tech Talk just in case? I mean before merging. |
Contributor
Author
Added item for next tech meeting |
Contributor
|
Maybe we should also remove it from the renovate rule: newrelic-agent-control/.github/renovate-shared-base.json Lines 94 to 99 in 0837997 |
Contributor
Author
Good catch! 54c8240 |
54c8240 to
6ca57c9
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In preparation for the incoming
kube-rsv4, we can start working to remove our dependency onserde_yaml!Summary
This PR replaces the deprecated
serde_yamldependency withserde_saphyracross the entire workspace, while migrating abstract YAML DOM manipulations fromserde_yaml::Value/Mapping/Numbertoserde_json::Value/Map/Number.Motivation
The
serde_yamlcrate is officially deprecated and no longer maintained.serde_saphyris the recommended modern replacement that provides YAML 1.2 serialization/deserialization viaserde.However,
serde_saphyrintentionally does not provideValue/Mapping/Numbertypes. All abstract YAML tree manipulations must be expressed throughserde_json::Value/Map/Numberinstead.Changes
Dependencies
serde_yamlfrom workspaceCargo.toml,agent-control/Cargo.toml, andtest/e2e-runner/Cargo.tomlserde-saphyras the replacement for serde operations (from_str,to_string,from_reader, etc.)serde_jsonfor abstract DOM types (Value,Map<String, Value>,Number)Source Migration (~66 files)
serde_yaml)serde_saphyr/serde_json)serde_yaml::Valueserde_json::Valueserde_yaml::Mappingserde_json::Map<String, serde_json::Value>serde_yaml::Numberserde_json::Numberserde_yaml::from_strserde_saphyr::from_strserde_yaml::to_stringserde_saphyr::to_stringserde_yaml::from_readerserde_saphyr::from_readerserde_yaml::to_valueserde_json::to_valueserde_yaml::from_valueserde_json::from_valueBehavioral Differences Handled
serde_saphyrhas stricter defaults thanserde_yaml. The following adjustments were made to preserve existing behavior:Duplicate YAML keys:
serde_saphyrerrors by default on duplicate mapping keys, whileserde_yamlsilently accepted them. Production code paths (FileStore::get,YAMLConfig::try_from) now useserde_saphyr::from_str_with_optionswithDuplicateKeyPolicy::LastWinsto match the previous behavior.Null → String deserialization:
serde_yamldeserializedname:(null value) into an emptyString.serde_saphyrrejects this. TheAgentTypeIDdeserialization was updated to useOption<String>fields with.unwrap_or_default()to handle this gracefully.Error message formats: Several unit tests that asserted on exact
serde_yamlerror strings were updated to assert on the equivalentserde_saphyrerror format.Block scalar serialization:
serde_saphyr::to_stringemits multiline strings as quoted strings with\nescapes rather than YAML literal block scalars (|). One integration test's expected output was updated accordingly.Error Enum Refactoring
Error variants were renamed to generic names not coupled to the old dependency:
serde_yaml::Errorvariants →serde_json::Errorandserde_saphyr::ser::Errorvariants with names likeSerialization/Deserializationagent_type/error.rs,k8s/error.rs,effective_agents_assembler.rs,config_migrate/migration/converter.rs, and othersTest Fixes
serde_saphyrerror message formatstest_store_remote_no_mocks) updated expected YAML serialization formatcustom_agent_type.rs(Map<String, Value>::new→Map::<String, Value>::new)Verification
cargo check --workspacepassescargo test -p newrelic_agent_control --lib— 863 tests passcargo test -p newrelic_agent_control --test integration_tests— 54 tests pass (76 ignored)cargo test --workspace --lib— all workspace lib tests passcargo clippy -p newrelic_agent_control— clean (1 pre-existing warning fixed)serde_yamlreferences in source code or testsBackward Compatibility
This change is intended to be behaviorally transparent to end users. All serialization/deserialization round-trips produce the same semantic results. The only user-visible difference is that YAML files with duplicate keys (which were previously silently accepted) are now also accepted via the
LastWinspolicy.Related
serde_yamldeprecation notice: https://github.com/dtolnay/serde_yaml/issues/388serde-saphyrcrate: https://crates.io/crates/serde-saphyrkube-rscrate switch: convert from serde-yaml to serde-saphyr kube-rs/kube#1975