Conversation
Piinks
changed the title
Piinks
changed the title
dnfield
left a comment
dnfield
left a comment
There was a problem hiding this comment.
A few questions - also, do you think you could split this into two PRs? One with the removing todos/improvement for commenting on drafts, and one with the changes for Skia gold?
You are right. 😝 |
|
Changes unrelated to Skia Gold have been put up in this PR: #413 |
|
All green now @dnfield PTAL. :) |
| break; | ||
| } | ||
| } | ||
| } catch(_) {} |
There was a problem hiding this comment.
What are we actually trying to catch here? We should generally avoid catch blocks that catch all exceptions, and also avoid catch blocks that swallow exceptions.
There was a problem hiding this comment.
I figured that if there is an error making the request it should just return false. If (let us hope never) Skia Gold is down and the request fails, I wouldn't want it to have a domino effect and break the bot as a result.
There was a problem hiding this comment.
I think in that case we should just try to catch SocketException and maybe HttpException, and just print something to the log that we couldn't reach Skia Gold.
We may also want to catch FormatException and print out the raw data we got and rethrow in case the response format changes.
There was a problem hiding this comment.
SGTM! I'll put it together! :)
There was a problem hiding this comment.
Or maybe just catch IOException instead of Socket/Http separately.
dnfield
left a comment
dnfield
left a comment
There was a problem hiding this comment.
LGTM!
Will you be deploying this?
Thanks! Can do! I haven't done that before - should be fun! |
2 participants
Skia Gold Support
This is part of the changes laid out in the design doc: flutter.dev/go/golden-workflow
GoldenTriageMessage:TODO