[app_dart] Differentiate 404s from other HTTP exceptions on GitHub files by CaseyHillers · Pull Request #1150 · flutter/cocoon

CaseyHillers · 2021-04-08T16:48:25Z

Refactor remoteFileContent to githubFileContent
- Only return response content if file was a 200 response
- Throw NotFoundException on 404s
- Throw HttpException on remaining exceptions
- Allows for customized responses where it may be okay if a config file does not exist, otherwise it should be a hard failure
If DeviceLab manifest.yaml 404s, return default response
Retrieve .ci.yaml and store it in cache for future use

Issues

CaseyHillers · 2021-04-08T16:50:54Z

app_dart/lib/src/foundation/utils.dart

Looking at the prod logs, I'm not sure devicelab/manifest.yaml and *_builders.json use the null response. I opt to make it a hard failure, but I can come back and fix them to return default responses if needed.

app_dart/lib/src/foundation/utils.dart

christopherfujino · 2021-04-08T21:45:50Z

app_dart/lib/src/foundation/utils.dart

In what situation would this happen? And should we trigger some sort of alert if we hit this?

This would only happen during a GitHub CDN outage (which we haven't faced issues with). The implementers of this function don't catch this error, so it would surface as an increased error rate in cocoon (we already use error tracking dashboards)

What happens if NotFoundException?

If NotFoundException, we should create an empty config. This case is expected when we remove the existing configs (devicelab/manifest.yaml, *_builders.json) or when a release branch is created without .ci.yaml.

keyonghan · 2021-04-08T23:43:04Z

app_dart/lib/src/foundation/utils.dart

What happens if NotFoundException?

app_dart/lib/src/foundation/utils.dart

keyonghan · 2021-04-08T23:56:19Z

app_dart/lib/src/service/scheduler.dart

Do we want to add a try catch block to unblock this API in case of exception?

Same for line 191 final YamlMap yaml = await loadDevicelabManifest(commit);

We retry HttpExceptions. I added back the fails safes for *_builder.json and .ci.yaml to not fail if we can't load those configs with logging to see how common it is.

app_dart/lib/src/service/scheduler.dart

keyonghan · 2021-04-09T00:03:43Z

app_dart/lib/src/service/scheduler.dart

Ditto. a / needed?

this is actually the format that githubFileContent expects.

I updated the format as if everything passes a leading slash, githubFileContent should just add it.

fluttergithubbot · 2021-04-09T06:34:01Z

This pull request is not suitable for automatic merging in its current state.

Please get at least one approved review before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

keyonghan

LGTM.

CaseyHillers · 2021-04-13T20:45:22Z

This has been at 50% of prod traffic for 4 hours, and I haven't seen any issues (other than unrelated auth warnings #1158). The HttpException flows require longer soak time, so i'll land this in the current state and follow up if we can remove the logic.

CaseyHillers commented Apr 8, 2021

View reviewed changes

CaseyHillers requested review from christopherfujino and keyonghan April 8, 2021 16:52

CaseyHillers mentioned this pull request Apr 8, 2021

Test all repo configs #1149

Merged

christopherfujino reviewed Apr 8, 2021

View reviewed changes

app_dart/lib/src/foundation/utils.dart Outdated Show resolved Hide resolved

christopherfujino reviewed Apr 8, 2021

View reviewed changes

Casey Hillers added 5 commits April 8, 2021 16:14

githubFileContent

1741560

download config

1f6253b

add devicelab 404 test

3e1897d

flutter analyze + fmt

e985b9d

review + rebase

94612b9

keyonghan reviewed Apr 9, 2021

View reviewed changes

godofredoc added the waiting for tree to go green Merge PR when tree becomes green via fluttergithubbot label Apr 9, 2021

fluttergithubbot removed the waiting for tree to go green Merge PR when tree becomes green via fluttergithubbot label Apr 9, 2021

temporarily catch http exceptions

d35d384

CaseyHillers force-pushed the ci-yaml-config branch from cf1f495 to d35d384 Compare April 9, 2021 17:42

keyonghan approved these changes Apr 13, 2021

View reviewed changes

CaseyHillers added the waiting for tree to go green Merge PR when tree becomes green via fluttergithubbot label Apr 13, 2021

fluttergithubbot merged commit 7c25bf0 into flutter:master Apr 13, 2021

CaseyHillers deleted the ci-yaml-config branch September 7, 2021 18:39

Conversation

CaseyHillers commented Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issues

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fluttergithubbot commented Apr 9, 2021

Uh oh!

keyonghan left a comment

Choose a reason for hiding this comment

Uh oh!

CaseyHillers commented Apr 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

CaseyHillers commented Apr 8, 2021 •

edited

Loading