Stop diffing blueprints and collections #7252
Conversation
|
This is the implementation of the first checkbox in #7240 |
| } | ||
|
|
||
| #[test] | ||
| fn test_initial() { |
There was a problem hiding this comment.
The only thing this test does is diff a blueprint and a collection, so it had to go.
| assert_eq!(diff.datasets.removed.len(), 0); | ||
| assert_eq!(diff.datasets.modified.len(), 0); | ||
|
|
||
| // The disposition has changed from `InService` to `Expunged` |
There was a problem hiding this comment.
This is actually a good change! The dataset was actually modified, we just couldn't see it with the reduction in form to the lowest common denominator shared with inventory that doesn't contain disposition.
There was a problem hiding this comment.
Is it worth adding a check on the contents of datasets.modified.first() (mostly for clarity)? E.g., the checks a few lines below this that confirm the modified zone is crucible and it was expunged.
| /// Information about a dataset as recorded in a blueprint | ||
| #[derive(Debug, Clone, Eq, PartialEq, JsonSchema, Deserialize, Serialize)] | ||
| pub struct BlueprintDatasetConfig { | ||
| // TODO: Display this in diffs - leave for now, for backwards compat |
There was a problem hiding this comment.
I'm trying to change as little of the diff output as possible with this change to demonstrate correctness.
In a follow up PR I'll add a column for disposition.
| assert_eq!(diff.datasets.removed.len(), 0); | ||
| assert_eq!(diff.datasets.modified.len(), 0); | ||
|
|
||
| // The disposition has changed from `InService` to `Expunged` |
There was a problem hiding this comment.
Is it worth adding a check on the contents of datasets.modified.first() (mostly for clarity)? E.g., the checks a few lines below this that confirm the modified zone is crucible and it was expunged.
| oxp_5fe54077-c016-49a9-becb-14993f133d43/crucible 2a41b700-5494-4aa1-900b-20618125c1d0 none none off | ||
| oxp_51c788ff-de33-43f7-b9c5-f5f56bf80736/crucible 0bbf7ff9-f6a6-49d8-8dc4-c75b2fdb5531 none none off | ||
| oxp_f52832ea-60d7-443b-9847-df5384bfc8e2/crucible cc43cb24-2e55-415d-9a2a-49a6b924b284 none none off | ||
| oxp_9825ff38-f07d-44a1-9efc-55a25e72015b/crucible 34de0f9d-ab3a-4047-bbf3-d2bf4beb2995 none none off |
There was a problem hiding this comment.
I tried to look at why this output changed. It looks like this list is sorted only by kind; within one kind, there doesn't appear to be any sorting. Maybe we can make this more stable - if this analysis looks right, maybe this fixes it?
--- a/nexus/types/src/deployment/blueprint_diff.rs
+++ b/nexus/types/src/deployment/blueprint_diff.rs
@@ -562,7 +562,7 @@ impl BpTableData for DiffDatasetsDetails {
// by sorting by dataset kind: after UUID redaction, that produces
// a stable table ordering for datasets.
let mut rows = self.datasets.values().collect::<Vec<_>>();
- rows.sort_unstable_by_key(|d| &d.kind);
+ rows.sort_unstable_by_key(|d| (&d.kind, &d.name));
rows.into_iter().map(move |dataset| {
BpTableRow::from_strings(state, dataset.as_strings())
})There was a problem hiding this comment.
I think this is a good change. It's going to result in a bit of churn in other output files though. I'd suggest looking at the coming commit standalone.
No description provided.