-
Notifications
You must be signed in to change notification settings - Fork 0
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
Ingest - tweaks following group demo #1202
Conversation
4decb1f
to
d56695e
Compare
d56695e
to
5a7d583
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1202 +/- ##
==========================================
+ Coverage 67.84% 68.60% +0.75%
==========================================
Files 110 109 -1
Lines 5847 5771 -76
Branches 641 634 -7
==========================================
- Hits 3967 3959 -8
+ Misses 1754 1685 -69
- Partials 126 127 +1 ☔ View full report in Codecov by Sentry. |
admin/ops/update_json_schemas.py
Outdated
|
||
import json | ||
from tempfile import TemporaryDirectory | ||
from pathlib import Path | ||
|
||
RECIPES_BUCKET = "edm-recipes" |
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.
Curious your rationale on adding a new bucket vs having all our schemas colocated. Just thinking that if we keep adding schemas, not sure they'll be 1-1 with buckets.
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.
Yeah, I agree with that, it just on one level made sense to have them in the same place as all the objects that refer to them. But I'm with you - this should really just live in one place
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.
And maybe we move it somewhere more "neutral" but that place can be publishing
for now
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.
fixed in new commit
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.
Looks great
Follow up to #1166. Primarily,