-
Notifications
You must be signed in to change notification settings - Fork 61
Internal cleanup for boot disk/instance update #6738
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Greg and Eliza were both right in comments on #6585, but since these are both fully internal I didn't want to add another CI round trip there :)
hawkw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's goooo!
| if params.create_params.boot_disk.is_none() { | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm...it occurs to me that it might be nicer to just change the make_saga_dag function to just not append a sic_set_boot_disk node if there is no boot disk param. Like, change this:
| builder.append(set_boot_disk_action()); |
to:
if params.create_params.boot_disk.is_some() {
builder.append(set_boot_disk_action());
}That way, we would never do the forward or reverse action if there's no boot disk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... huh, i suppose that makes sense. i'd read something in one of the comments here about Steno not handling dynamically-shaped sagas, but the comment earlier about MAX_NICS_PER_INSTANCE makes me think i'd read something old without realizing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm...a saga's DAG can definitely depend on its params, so it's possible that the comment either meant something else by "dynamically-shaped", or was just out of date...
gjcolombo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with a +1 to @hawkw's suggestion to just omit the set-boot-disk node from the create saga if the params don't specify that it's required.
Greg and Eliza were both right in comments on #6585, but since these are both fully internal I didn't want to add another CI round trip there :)