[nexus] Allow specifying anti-affinity groups during instance creation#7864
Conversation
|
|
||
| /// Affinity groups which this instance should be added into. | ||
| #[serde(default)] | ||
| pub affinity_groups: Vec<NameOrId>, |
There was a problem hiding this comment.
If affinity groups are considered experimental, should we leave them out here at first?
There was a problem hiding this comment.
Yeah, good idea. I'll just delete this portion of the code for now.
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
not_found_by_id and not_found_by_name might make this very slightly shorter. Also you could loop through the names and IDs vecs since you've already done the match to separate them above. Neither change is necessary though, this is all clear enough.
There was a problem hiding this comment.
Sure thing, I went ahead and made both these changes.
| .iter() | ||
| .map(|id| NameOrId::Id(id.into_untyped_uuid())) | ||
| .collect::<Vec<NameOrId>>()) | ||
| } |
There was a problem hiding this comment.
Worth having these return just Vec<Uuid> since they're all IDs? I'm guessing that would require changes downstream that might be annoying, though it could also let you delete some annoying match statements.
There was a problem hiding this comment.
I would like to - this what my TODO comment is about below. Right now, InstanceCreate is a big struct of unvalidated user parameters, and we do perform validation on it, but don't transform the type before handing it to the saga machinery. I think we should transform the type (hence the TODO) but it needs to be a NameOrId for now.
| "Non-UUID group".to_string(), | ||
| )); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Yeah, if they're all IDs you don't even need this match.
There was a problem hiding this comment.
Agreed, this will require a new struct, and InstanceCreate already needs some clean-up. I can do this, but I'm probably going to save it for a follow-up PR?
david-crespo
left a comment
There was a problem hiding this comment.
Looks good to me. My comments are just suggestions. I think the API is good, don't really see what else it could be.
Fixes #7723