Updated chart data, S3 aggregate parquet moved #124
Conversation
dogversioning
force-pushed
the
mg/chart_data_update
branch
from
eade41e
to
8e5589e
Compare
☂️ Python Coverage
Overall Coverage
New Files
Modified Files
|
docs/dashboard_api.prod.yaml
Outdated
| @@ -580,12 +577,23 @@ paths: | |||
| content: {} | |||
| security: | |||
| - api_key: [] | |||
| /study-periods: | |||
| /data_packages/{data_package_id}: | |||
There was a problem hiding this comment.
nit: this is so lonely at the end, separated from its child API
There was a problem hiding this comment.
This is autogenerated from API gateway, I have no control over it.
| res = functions.http_response(200, res) | ||
| except errors.AggregatorS3Error: | ||
| res = functions.http_response( | ||
| 404, f"Aggregate for {path_params['data_package_id']} not found" |
There was a problem hiding this comment.
nit: that's a very specific conclusion to draw from a generic "something happened error". Might be helpful to log the original error message and/or include it here too.
There was a problem hiding this comment.
The error should get logged to cloudwatch, security asked us to have no stack traces in responses and to generally have these error messages be unhelpful while the API is externally accessible.
There was a problem hiding this comment.
Maybe add a comment to that effect? Like... if a third developer comes across this in 2 years, they won't have that context. Something like "# don't be too informative about internal state in our error messages" (but in Matt voice) above this generic response.
There was a problem hiding this comment.
ok, added a brief note.
/chart_data/endpoint to/data_packages/[data_package_id]/chart