[ENH] Multi-region, multi-cloud configuration. #6092
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
|
Introduce multi-cloud topology configuration types Adds a new Key Changes• Introduced Affected Areas• rust/types/src/topology.rs This summary was automatically generated by @propel-code-bot |
![propel-code-bot[bot]](https://avatars.githubusercontent.com/in/1179004?s=60&v=4){kind=small}
| serialize = "T: Clone + Debug + Eq + PartialEq + Serialize", | ||
| deserialize = "T: Clone + Debug + Eq + PartialEq + serde::de::DeserializeOwned" | ||
| ))] | ||
| pub struct ProviderRegion<T: Clone + Debug + Eq + PartialEq + Serialize + for<'a> Deserialize<'a>> { |
There was a problem hiding this comment.
[Maintainability] For consistency and clarity, consider using serde::de::DeserializeOwned as the trait bound for T instead of for<'a> Deserialize<'a>. The manual Deserialize implementation for MultiCloudMultiRegionConfiguration already uses DeserializeOwned, and since T represents owned configuration data, this bound is more idiomatic.
This change would also apply to the MultiCloudMultiRegionConfiguration struct definition and associated impl blocks.
Context for Agents
For consistency and clarity, consider using `serde::de::DeserializeOwned` as the trait bound for `T` instead of `for<'a> Deserialize<'a>`. The manual `Deserialize` implementation for `MultiCloudMultiRegionConfiguration` already uses `DeserializeOwned`, and since `T` represents owned configuration data, this bound is more idiomatic.
This change would also apply to the `MultiCloudMultiRegionConfiguration` struct definition and associated `impl` blocks.
File: rust/types/src/topology.rs
Line: 177| fn format_message(&self) -> String { | ||
| if !self.has_errors() { | ||
| return "no validation errors".to_string(); | ||
| } | ||
|
|
||
| let mut parts = Vec::new(); | ||
|
|
||
| if !self.duplicate_region_names.is_empty() { | ||
| let names: Vec<&str> = self | ||
| .duplicate_region_names | ||
| .iter() | ||
| .map(|n| n.as_str()) | ||
| .collect(); | ||
| parts.push(format!("duplicate region names: {}", names.join(", "))); | ||
| } | ||
|
|
||
| if !self.duplicate_topology_names.is_empty() { | ||
| let names: Vec<&str> = self | ||
| .duplicate_topology_names | ||
| .iter() | ||
| .map(|n| n.as_str()) | ||
| .collect(); | ||
| parts.push(format!("duplicate topology names: {}", names.join(", "))); | ||
| } | ||
|
|
||
| if !self.unknown_topology_regions.is_empty() { | ||
| let names: Vec<&str> = self | ||
| .unknown_topology_regions | ||
| .iter() | ||
| .map(|n| n.as_str()) | ||
| .collect(); | ||
| parts.push(format!("unknown topology regions: {}", names.join(", "))); | ||
| } | ||
|
|
||
| if let Some(ref name) = self.unknown_preferred_region { | ||
| parts.push(format!("unknown preferred region: {}", name.as_str())); | ||
| } | ||
|
|
||
| parts.join("; ") | ||
| } | ||
| } |
There was a problem hiding this comment.
[Maintainability] The format_message function contains repetitive logic for formatting lists of names. This can be extracted into a private helper function to improve code clarity and maintainability by following the DRY (Don't Repeat Yourself) principle. This also provides an opportunity to use the Display implementation directly instead of as_str().
Context for Agents
The `format_message` function contains repetitive logic for formatting lists of names. This can be extracted into a private helper function to improve code clarity and maintainability by following the DRY (Don't Repeat Yourself) principle. This also provides an opportunity to use the `Display` implementation directly instead of `as_str()`.
File: rust/types/src/topology.rs
Line: 498
Description of changes
This PR introduces the types for multi-region, multi-cloud support.
Test plan
CI
Migration plan
Not applicable yet.
Observability plan
N/A
Documentation Changes
N/A