-
Notifications
You must be signed in to change notification settings - Fork 18
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
feat: report patch install failure events #78
Conversation
This doesn't tell us anything about the failure, just that it happened. Which at least will let us communicate that to customers. This currently has no tests and will need some before landing.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #78 +/- ##
==========================================
+ Coverage 97.47% 97.50% +0.03%
==========================================
Files 10 10
Lines 1858 2043 +185
==========================================
+ Hits 1811 1992 +181
- Misses 47 51 +4
☔ View full report in Codecov by Sentry. 📢 Have feedback on the report? Share it here. |
It's only a partial test, but I think it's good enough to review. We'll do some manual testing too. |
UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version); | ||
// This will clear any events which got queued between the time we | ||
// loaded the state now, but that's OK for now. | ||
let result = state.clear_events(); |
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.
How does this change get saved?
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.
pub fn clear_events(&mut self) -> Result<()> {
self.queued_events.clear();
self.save()
}```
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.
Oh interesting. I wonder if the "save at the end of a mutating method" pattern is something we want to either adopt throughout or discourage, as it's currently done inconsistently in UpdaterState
.
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.
Yeah, the inconsistency is the problem. I suspect what we actually want is some sort of file system object which you have to get a lock to use? And then UpdaterState doesnt' know how to save itself, rather it gets passed to something that knows how to read or write it?
@@ -367,7 +393,22 @@ pub fn report_launch_failure() -> anyhow::Result<()> { | |||
)))?; | |||
// Ignore the error here, we'll try to activate the next best patch | |||
// even if we fail to mark this one as bad (because it was already bad). | |||
let _ = state.mark_patch_as_bad(patch.number); | |||
let mark_result = state.mark_patch_as_bad(patch.number); |
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.
Same Q here re: saving state
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.
It gets accidentally saved below by the queue_patch call.
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.
Sorry by the activate_latest_bootable_patch() call.
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.
👍 We should at least document that that's what is happening, as the inconsistency of whether state
's mutable methods trigger a save is a bit confusing.
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.
I added a doc about where the save happens. Would you like others? Happy to, just would like inspiration as to what docs you're seeking.
@@ -367,7 +393,22 @@ pub fn report_launch_failure() -> anyhow::Result<()> { | |||
)))?; | |||
// Ignore the error here, we'll try to activate the next best patch | |||
// even if we fail to mark this one as bad (because it was already bad). | |||
let _ = state.mark_patch_as_bad(patch.number); | |||
let mark_result = state.mark_patch_as_bad(patch.number); |
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.
👍 We should at least document that that's what is happening, as the inconsistency of whether state
's mutable methods trigger a save is a bit confusing.
I think we/I should take a separate pass to make the saving more explicit/sane. |
This doesn't tell us anything about the failure, just that it
happened. Which at least will let us communicate that to customers.
This currently has no tests and will need some before landing.