-
-
Notifications
You must be signed in to change notification settings - Fork 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.
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
Added support for temporary table upload in alma
#3118
Conversation
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 like a nice addition, just needs a little clearer documentation - can you provide an example upload table in the narrative docs?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3118 +/- ##
==========================================
- Coverage 67.39% 67.37% -0.02%
==========================================
Files 233 233
Lines 18421 18410 -11
==========================================
- Hits 12415 12404 -11
Misses 6006 6006 ☔ View full report in Codecov by Sentry. |
Not sure what the problem is now. There's 100% patch coverage. |
Ignore that, coverage number doesn't take remote tests into account. |
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.
Overall looks good to me. I'll leave the actual approval and merge to Adam though.
One minor comment, that I see a bunch of test failures in the docs, all of those are present on main
, too but it would be nice to eventually clean them up.
0970283
to
d6363c6
Compare
Co-authored-by: Adam Ginsburg <[email protected]> Co-authored-by: Adam Ginsburg <[email protected]>
alma
Can this be merged or do I need to try to fix the failing check (which I have no idea why is failing)? Thanks |
No, coverage is a red herring (it doesn't take into account the remote coverage and anyways likes to act up). I was waiting to give Adam a chance to chime in, but I suppose it's time to get this merged. Thanks! |
Yep, sorry, I was happy with this. |
Thank you both. |
Solution for #3030
Also cleaned up some of the alma remote tests to make them a little bit faster.