-
Notifications
You must be signed in to change notification settings - Fork 212
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
Fix image proxy get error handling #3455
Conversation
tallies_incr( | ||
f"thumbnail_http_error:{domain}:{month}:{code}:{exc.response.text}" | ||
) | ||
tallies_incr(f"thumbnail_http_error:{domain}:{month}:{status}") |
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.
The response text is ambiguous from aiohttp (I wasn't entirely sure what it was and whether it matched wherever requests as using). I removed it because I figure we really care more about the status code, and if we need specific details about the error itself, we can look at the logs for the exc.message or review the captured exception in sentry (if relevant).
@@ -33,9 +33,9 @@ def test_health_check_calls__check_db(api_client): | |||
|
|||
def test_health_check_es_timed_out(api_client): | |||
mock_health_response(timed_out=True) | |||
pook.on() |
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.
This and the other changes in this test module are cleanup. Using pook.on
and pook.off
like this means that if api_client.get
results in an exception (which is a test failure), then pook would never get turned off. It's exactly what a context manager is for anyway.
@@ -21,7 +21,7 @@ x-airflow-common: &airflow-common | |||
- CATALOG_PY_VERSION | |||
- CATALOG_AIRFLOW_VERSION | |||
volumes: | |||
- ./catalog:/opt/airflow/catalog | |||
- ./catalog:/opt/airflow/catalog:z |
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.
These changes are for #3276
03e1839
to
8267170
Compare
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.
Changes work well. Nice cleaning up of the code and tests!
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.
LGTM!
Fixes
Fixes #3454 by @sarayourfriend
Also fixes #3276 because I really need to stop fixing this every time I spin the app up locally after a week of not running it.
Description
Replaces the error handling code specific to requests exceptions with aiohttp handling. Just something I missed in code and we missed in reviewing #3388.
Testing Instructions
Review the code changes and confirm I've covered the cases. I had to make a change to one of the error keys but I'll explain it in a comment on that line in the PR.
Tests and CI should pass, of course!
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin