database support for PendingMgsUpdate for SP updates
#8291
Conversation
| // Right now, we only implement support for storing SP updates. | ||
| let PendingMgsUpdateDetails::Sp { |
There was a problem hiding this comment.
Nit - maybe match here instead of let ... else ...? That way it's obvious which cases aren't implemented yet.
| // XXX-dap size of int | ||
| SpMgsSlot::from(SqlU16::from(u16::try_from(update.slot_id).unwrap())).into_sql::<diesel::sql_types::Int4>(), |
There was a problem hiding this comment.
Should we change PendingMgsUpdate::slot_id to u16 to not have to do this?
There was a problem hiding this comment.
Yeah, I need to revisit this. I feel like I looked at this when I was first defining this struct and I picked u32 because some other place provided that. I need to go try that to remember where that was.
There was a problem hiding this comment.
I looked into doing this and decided to split it into two PRs.
- one to change
PendingMgsUpdate::slot_idto u16 - one to change MGS to expose the slot ids as u16
I'll do these in follow-ons. For now I have added an unwrap() but it will be short-lived (and I've filed #8378 to track it).
| .execute_async(&conn) | ||
| .await?; | ||
| if count != 1 { | ||
| // This should be impossible in practice. We will insert |
There was a problem hiding this comment.
Is this what you were referring to in the PR description ("cleanup -- the insertion code is really gross")?
There was a problem hiding this comment.
I was thinking of L471-L485. I believe that code is doing the right thing, but it's pretty gnarly to read. I figured I'd add some comments, extract some of the expressions into named variables, and just edit a bit to see how I can make it more readable.
|
The cleanup really wanted a rustfmt pass, but rustfmt hasn't been working on this code path. Fixing that added a bunch of noise so I split that into #8394 and made this PR dependent on that one. |
This is mostly code-complete, the test is updated, and it appears to work.
Depends on #8394.
TODO: