-
Notifications
You must be signed in to change notification settings - Fork 671
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
[flyteadmin] Use WithContext in all DB calls #5538
Conversation
Signed-off-by: Jason Parraga <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5538 +/- ##
=======================================
Coverage 60.97% 60.98%
=======================================
Files 796 796
Lines 51647 51647
=======================================
+ Hits 31492 31497 +5
+ Misses 17255 17250 -5
Partials 2900 2900
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
thank you for this! do you need any help preparing this PR to move it from draft and ready for review? |
I was going to wait for CI to run but it looks like it has now so feel free to review. |
great, thank you! re-running the timed-out test now re:
I'd be curious if you could share what RBAC work you're doing? |
I added RBAC to our fork so that we can restrict API access as well do project/domain level resource filtering/isolation. The filtering is most elegantly done at the data layer so that pagination works nicely. |
neat thanks for sharing! how are you mapping user identity to permitted projects and domains (also happy to continue this conversation on OSS slack if you're around) |
Signed-off-by: Jason Parraga <[email protected]> Signed-off-by: Vladyslav Libov <[email protected]>
I have been working in the DB code to implement some RBAC and I noticed some TODOs about using
db.WithContext
. I audited the code and addeddb.WithContext
for any missing DB methods. This should ensure that all DB calls support distributed tracing.Setup process
Screenshots
Check all the applicable boxes