-
Notifications
You must be signed in to change notification settings - Fork 97
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
Rename database APIs #8143
Rename database APIs #8143
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8143 +/- ##
==========================================
+ Coverage 59.47% 59.49% +0.01%
==========================================
Files 577 580 +3
Lines 38644 39008 +364
==========================================
+ Hits 22984 23207 +223
- Misses 13994 14085 +91
- Partials 1666 1716 +50 ☔ View full report in Codecov by Sentry. |
This change renames the database APIs to have clearer names. Signed-off-by: Ryan Nowak <[email protected]>
757e5b3
to
2f319af
Compare
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
@@ -26,7 +26,7 @@ import ( | |||
runtimelog "sigs.k8s.io/controller-runtime/pkg/log" | |||
|
|||
"github.com/radius-project/radius/pkg/armrpc/hostoptions" | |||
"github.com/radius-project/radius/pkg/ucp/dataprovider" | |||
"github.com/radius-project/radius/pkg/ucp/databaseprovider" |
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.
Do we have both database and databaseprovider folders under ucp? I think this is the best practice, right? I am asking because I would create database under ucp and then provider under database.
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.
I'm planning to rearrange these in the next PR 👍
I think it's a big improvement to name the package databaseprovider
instead of provider
. You'll notice that everywhere we reference these, they get aliased to something else.
queue "github.com/radius-project/radius/pkg/ucp/queue/client" | ||
qprovider "github.com/radius-project/radius/pkg/ucp/queue/provider" | ||
"github.com/radius-project/radius/pkg/ucp/queue/queueprovider" |
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.
Like here the queueprovider is under ucp/queue. I believe that this can be renamed to ucp/queue/provider but it is not in the scope of this PR.
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.
I'm planning to rearrange these in the next PR 👍
"github.com/radius-project/radius/pkg/ucp/databaseprovider" | ||
"github.com/radius-project/radius/pkg/ucp/queue/queueprovider" | ||
"github.com/radius-project/radius/pkg/ucp/secret/secretprovider" |
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.
So, we renamed the queue/provider and secret/provider to queue/queueprovider and secret/secretprovider but kept databaseprovider under ucp. 🤨
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.
I'm planning to rearrange these in the next PR 👍
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Description
This change renames the database APIs to have clearer names.
Type of change
Contributor checklist
Please verify that the PR meets the following requirements, where applicable: