Put BlueprintZoneConfig into a map
#7315
Conversation
| @@ -153,29 +149,12 @@ impl ZonesEditor { | |||
| } | |||
| } | |||
|
|
|||
| impl TryFrom<BlueprintZonesConfig> for ZonesEditor { | |||
There was a problem hiding this comment.
I was happy to see this go away :)
There was a problem hiding this comment.
Yeah, getting rid of this is excellent. 👍
This is a largely mechanical change to store zones in a Blueprint mapped by their zone uuid rather than in a Vec. This is primarily to support diffs of `BlueprintZonesConfig` via diffus. Diffus doesn't handle Vecs in a manner where inserts, removals, and modifications are tracked based on an id. To minimize the PR diff, the zone id was not removed from `BlueprintZoneConfig`. The key to the map must always match the value in `BlueprintZoneConfig`. It's a shame a mismatch is now representable, and we can go about trying to change that if necessary. We can also just make it part of blippy for now. FWIW, this matches the pattern in `BlueprintDatasetsConfig`. Putting the `BlueprintZoneConfig` into a map means we no longer require the sort method on `BlueprintZonesConfig` as the order is stable now. However, we still want to ensure the same display sort order as previously, so we make sure to sort things properly before creating the table. We'll want similar changes for `BlueprintPhysicalDisksConfig`. That can come in a follow up PR.
andrewjstone
force-pushed
the
blueprint-zones-map
branch
from
e033cd2 to
5eb8178
Compare
There was a problem hiding this comment.
Changes LGTM, other than a nit about some copy/paste hazards.
The key to the map must always match the value in BlueprintZoneConfig. It's a shame a mismatch is now representable, and we can go about trying to change that if necessary. We can also just make it part of blippy for now. FWIW, this matches the pattern in BlueprintDatasetsConfig.
Agreed that the potential for this mismatch is unfortunate. I also have some vague worry about accidental ID reuse. If we use a Vec and reuse IDs, we don't realize it until we try to convert it to a map; if we use a BTreeMap and reuse IDs, we only realize it if we're consistent with checking the return value of insert() (which seems like a tall order). That said, I think the odds of either kind of error (wrong key or duplicate IDs) showing up in practice is kind of low, and you're right that blippy could at least check for the "wrong key" case.
Do you think it would be worth having our own KeyedMap type? I'm imagining something like KeyedMap<BlueprintZoneConfig> that internally is a wrapper around BTreeMap<OmicronZoneUuid, BlueprintZoneConfig> that enforces the invariants we want w.r.t. keys and duplicates. This sounds kind of appealing; I might take a stab at it if you agree. (It should be easy to make it reusable for datasets / disks too.)
| id: external_dns_id.into_untyped_uuid(), | ||
| zones: [ | ||
| ( | ||
| external_dns_id, |
There was a problem hiding this comment.
Manually duplicating the IDs seems a little dicey or easy to mess up when copy/pasting. Could we do something like
zones: [
// all the BlueprintZoneConfigs
].into_iter().map(|z| (z.id, z)).collect()instead to all the instances of this pattern? (Looks like it shows up a few times in this file and a few times in other files)
There was a problem hiding this comment.
Good idea! I should have thought of this :D
There was a problem hiding this comment.
Hah, that change shrunk the diff quite a bit.
| db_nic_from_zone( | ||
| bp1.blueprint_zones[&sled_ids[2]] | ||
| .zones | ||
| .first_key_value() |
There was a problem hiding this comment.
TIL first_key_value() (and first_entry(), huh!). Thanks!
| @@ -153,29 +149,12 @@ impl ZonesEditor { | |||
| } | |||
| } | |||
|
|
|||
| impl TryFrom<BlueprintZonesConfig> for ZonesEditor { | |||
There was a problem hiding this comment.
Yeah, getting rid of this is excellent. 👍
I like it! It solves the problem at hand and it allows us to keep the id in the value, which has proven useful in past lives. |
This is a largely mechanical change to store zones in a Blueprint mapped by their zone uuid rather than in a Vec. This is primarily to support diffs of
BlueprintZonesConfigvia diffus. Diffus doesn't handle Vecs in a manner where inserts, removals, and modifications are tracked based on an id.To minimize the PR diff, the zone id was not removed from
BlueprintZoneConfig. The key to the map must always match the value inBlueprintZoneConfig. It's a shame a mismatch is now representable, and we can go about trying to change that if necessary. We can also just make it part of blippy for now. FWIW, this matches the pattern inBlueprintDatasetsConfig.Putting the
BlueprintZoneConfiginto a map means we no longer require the sort method onBlueprintZonesConfigas the order is stable now. However, we still want to ensure the same display sort order as previously, so we make sure to sort things properly before creating the table.We'll want similar changes for
BlueprintPhysicalDisksConfig. That can come in a follow up PR.