-
Notifications
You must be signed in to change notification settings - Fork 929
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
Switch primaryDatasource -> datasources #7678
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.
super nit: should we make it a helper func? I know it's one line of code but could be findPrimaryDataSource(schema).postgresql....
LGTM either way
@@ -262,16 +262,17 @@ async function promptForService( | |||
info.serviceId = serviceName.serviceId; | |||
info.locationId = serviceName.location; | |||
if (choice.schema) { | |||
if (choice.schema.primaryDatasource.postgresql?.cloudSql.instance) { | |||
const primaryDatasource = choice.schema.datasources.find((d) => d.postgresql); |
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.
nit: Can multiple datasources specify postgresql
? Also given that the backend resource doesn't specify a "primary" datasource anymore -- is the "primary" datasource intended to just be the first in the list?
Maybe for now we can just pull the first datasource for now and verify it has a postgresql
field?
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.
This is definitely a short sighted solution on my end, because we will definitely need a pretty major overhaul of the init code once we support multiple datasources. Find only grabs the first item in the list for which the function returns true, so right now this just grabs the first postgres datasource.
src/deploy/dataconnect/deploy.ts
Outdated
const databaseId = s.schema.primaryDatasource.postgresql?.database; | ||
if (!instanceId || !databaseId) { | ||
return Promise.resolve(); | ||
const primaryDatasource = s.schema.datasources.find((d) => d.postgresql); |
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.
nit: For future-proofing (since the "primary" datasource won't necessarily be the Postgres one) would it make more sense to name this postgresDatasource
?
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.
Good call, will rename
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7678 +/- ##
==========================================
- Coverage 56.91% 56.84% -0.08%
==========================================
Files 396 396
Lines 27599 27652 +53
Branches 5692 5709 +17
==========================================
+ Hits 15708 15718 +10
- Misses 11723 11765 +42
- Partials 168 169 +1 ☔ View full report in Codecov by Sentry. |
Description
Switching primaryDatasource -> datasources, as part of the move to v1beta apis.