Skip to content
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

Merged
merged 1 commit into from
Jul 4, 2024

Conversation

Sovietaced
Copy link
Contributor

@Sovietaced Sovietaced commented Jul 4, 2024

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 added db.WithContext for any missing DB methods. This should ensure that all DB calls support distributed tracing.

Setup process

Screenshots

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Signed-off-by: Jason Parraga <[email protected]>
Copy link

codecov bot commented Jul 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.98%. Comparing base (e13bfe3) to head (6e1a91d).
Report is 109 commits behind head on master.

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           
Flag Coverage Δ
unittests-datacatalog 69.31% <ø> (ø)
unittests-flyteadmin 58.73% <100.00%> (+0.04%) ⬆️
unittests-flytecopilot 17.79% <ø> (ø)
unittests-flytectl 67.44% <ø> (ø)
unittests-flyteidl 79.06% <ø> (ø)
unittests-flyteplugins 61.85% <ø> (ø)
unittests-flytepropeller 57.42% <ø> (ø)
unittests-flytestdlib 65.65% <ø> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@katrogan
Copy link
Contributor

katrogan commented Jul 4, 2024

thank you for this! do you need any help preparing this PR to move it from draft and ready for review?

@Sovietaced Sovietaced marked this pull request as ready for review July 4, 2024 08:10
@Sovietaced
Copy link
Contributor Author

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.

@katrogan
Copy link
Contributor

katrogan commented Jul 4, 2024

great, thank you! re-running the timed-out test now

re:

DB code to implement some RBAC

I'd be curious if you could share what RBAC work you're doing?

@katrogan katrogan merged commit b63ce0e into flyteorg:master Jul 4, 2024
50 checks passed
@Sovietaced
Copy link
Contributor Author

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.

@katrogan
Copy link
Contributor

katrogan commented Jul 5, 2024

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)

vlibov pushed a commit to vlibov/flyte that referenced this pull request Aug 16, 2024
Signed-off-by: Jason Parraga <[email protected]>
Signed-off-by: Vladyslav Libov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants