Skip to content

Skia Gold support #409

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.

Sign up for GitHub

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

Jump to bottom
Merged
Piinks merged 30 commits into from
Sep 17, 2019
Merged

Skia Gold support #409

Piinks merged 30 commits into from
Sep 17, 2019

Conversation

@Piinks
Copy link
Contributor

@Piinks Piinks commented Sep 11, 2019 •
edited
Loading

Skia Gold Support

This is part of the changes laid out in the design doc: flutter.dev/go/golden-workflow

GoldenTriageMessage:

Nice merge! 🎉
It looks like this PR made changes to golden files. Be sure to visit Flutter Gold to triage the results when post-submit testing has completed. The status of these tests can be seen on the Flutter Dashboard.

For more information about working with golden files, see the wiki page Writing a Golden File Test for package:flutter/flutter.

TODO

  • Update wiki to provide guidance on Skia Gold triage

@Piinks Piinks changed the title WIP Skia Gold support & other improvements WIP Skia Gold support & github_webhook clean-up Sep 12, 2019
@Piinks Piinks mentioned this pull request Sep 12, 2019
Closed
11 tasks
@Piinks Piinks marked this pull request as ready for review September 12, 2019 19:13
@Piinks Piinks changed the title WIP Skia Gold support & github_webhook clean-up Skia Gold support & github_webhook clean-up Sep 12, 2019
@Piinks Piinks requested a review from dnfield September 12, 2019 19:15
dnfield
dnfield reviewed Sep 12, 2019
View reviewed changes
Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@Piinks
Copy link
Contributor Author

Piinks commented Sep 12, 2019

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. 😝
I will split these up.

@Piinks Piinks mentioned this pull request Sep 12, 2019
Closed
@Piinks Piinks changed the title Skia Gold support & github_webhook clean-up Skia Gold support Sep 12, 2019
@Piinks
Copy link
Contributor Author

Piinks commented Sep 12, 2019

Changes unrelated to Skia Gold have been put up in this PR: #413
I recommend landing that first and then merging changes. :)

@Piinks Piinks changed the title Skia Gold support WIP Skia Gold support Sep 13, 2019
Piinks
Piinks commented Sep 13, 2019
View reviewed changes
@Piinks Piinks changed the title WIP Skia Gold support Skia Gold support Sep 16, 2019
@Piinks
Copy link
Contributor Author

Piinks commented Sep 16, 2019

All green now @dnfield PTAL. :)

dnfield
dnfield reviewed Sep 16, 2019
View reviewed changes
app_dart/lib/src/request_handlers/github_webhook.dart Outdated
break;
}
}
} catch(_) {}
Copy link
Contributor

@dnfield dnfield Sep 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@Piinks Piinks Sep 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@dnfield dnfield Sep 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@Piinks Piinks Sep 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM! I'll put it together! :)

Copy link
Contributor

@dnfield dnfield Sep 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe just catch IOException instead of Socket/Http separately.

@Piinks Piinks mentioned this pull request Sep 16, 2019
Merged
8 tasks
dnfield
dnfield approved these changes Sep 17, 2019
View reviewed changes
Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Will you be deploying this?

@Piinks
Copy link
Contributor Author

Piinks commented Sep 17, 2019

LGTM!

Will you be deploying this?

Thanks! Can do! I haven't done that before - should be fun!

@Piinks Piinks merged commit e60b1e6 into flutter:master Sep 17, 2019
@Piinks Piinks mentioned this pull request Sep 17, 2019
Merged
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@dnfield dnfield dnfield approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Piinks @dnfield