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.

Sign up for GitHub

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

Jump to bottom

Switch primaryDatasource -> datasources #7678

Merged
merged 5 commits into from
Sep 18, 2024
Merged

Switch primaryDatasource -> datasources #7678

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2a05791
Switch primaryDatasource -> datasources
joehan Sep 18, 2024
ded0dc7
?.
joehan Sep 18, 2024
bdf82e2
Merge branch 'master' into jh-primaryDS
joehan Sep 18, 2024
70ffa6a
primaryDatasource -> postgresDatasource
joehan Sep 18, 2024
fb6b746
formats
joehan Sep 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion scripts/dataconnect-test/tests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
const FIREBASE_PROJECT = process.env.FBTOOLS_TARGET_PROJECT || "";
const FIREBASE_DEBUG = process.env.FIREBASE_DEBUG || "";

function expected(

Check warning on line 14 in scripts/dataconnect-test/tests.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Missing return type on function
serviceId: string,
databaseId: string,
schemaUpdateTime: string,
Expand All @@ -31,14 +31,14 @@
};
}

async function cleanUpService(projectId: string, serviceId: string, databaseId: string) {

Check warning on line 34 in scripts/dataconnect-test/tests.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Missing return type on function
await client.deleteServiceAndChildResources(
`projects/${projectId}/locations/us-central1/services/${serviceId}`,
);
await deleteDatabase(projectId, "dataconnect-test", databaseId);
}

async function list() {

Check warning on line 41 in scripts/dataconnect-test/tests.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Missing return type on function
return await cli.exec(
"dataconnect:services:list",
FIREBASE_PROJECT,
Expand All @@ -51,7 +51,7 @@
);
}

async function migrate(force: boolean) {

Check warning on line 54 in scripts/dataconnect-test/tests.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Missing return type on function
const args = force ? ["--force"] : [];
if (FIREBASE_DEBUG) {
args.push("--debug");
Expand All @@ -68,7 +68,7 @@
);
}

async function deploy(force: boolean) {

Check warning on line 71 in scripts/dataconnect-test/tests.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Missing return type on function
const args = ["--only", "dataconnect"];
if (force) {
args.push("--force");
Expand All @@ -81,7 +81,7 @@
});
}

function toPath(p: string) {

Check warning on line 84 in scripts/dataconnect-test/tests.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Missing return type on function
return path.join(__dirname, p);
}

Expand Down Expand Up @@ -124,7 +124,7 @@
return { serviceId, databaseId };
}

function prepareStep(step: Step) {

Check warning on line 127 in scripts/dataconnect-test/tests.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Missing return type on function
fs.writeFileSync(toPath("fdc-test/schema/schema.gql"), step.schemaGQL, { mode: 420 /* 0o644 */ });
fs.writeFileSync(toPath("fdc-test/connector/connector.gql"), step.connectorGQL, {
mode: 420 /* 0o644 */,
Expand Down Expand Up @@ -158,8 +158,8 @@
await deploy(false);
await migrate(true);
await deploy(true);
} catch (err: any) {

Check warning on line 161 in scripts/dataconnect-test/tests.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Unexpected any. Specify a different type
expect(err.expectErr, `Unexpected error: ${err.message}`).to.be.true;

Check warning on line 162 in scripts/dataconnect-test/tests.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Unsafe member access .expectErr on an `any` value

Check warning on line 162 in scripts/dataconnect-test/tests.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Invalid type "any" of template literal expression
}
expect(step.expectErr).to.be.false;
const result = await list();
Expand All @@ -173,7 +173,7 @@
serviceId,
databaseId,
service["schemaUpdateTime"],
service["connectors"][0]["connectorLastUpdated"],
service["connectors"]?.[0]?.["connectorLastUpdated"],
),
);
}
Expand Down
7 changes: 4 additions & 3 deletions src/commands/dataconnect-services-list.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,15 @@ export const command = new Command("dataconnect:services:list")
for (const service of services) {
const schema = (await client.getSchema(service.name)) ?? {
name: "",
primaryDatasource: {},
datasources: [{}],
source: { files: [] },
};
const connectors = await client.listConnectors(service.name);
const serviceName = names.parseServiceName(service.name);
const instanceName = schema?.primaryDatasource.postgresql?.cloudSql.instance ?? "";
const postgresDatasource = schema?.datasources.find((d) => d.postgresql);
const instanceName = postgresDatasource?.postgresql?.cloudSql.instance ?? "";
const instanceId = instanceName.split("/").pop();
const dbId = schema?.primaryDatasource.postgresql?.database ?? "";
const dbId = postgresDatasource?.postgresql?.database ?? "";
const dbName = `CloudSQL Instance: ${instanceId}\nDatabase: ${dbId}`;
table.push([
serviceName.serviceId,
Expand Down
8 changes: 3 additions & 5 deletions src/dataconnect/load.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,9 @@ export async function load(
sourceDirectory: resolvedDir,
schema: {
name: `${serviceName}/schemas/${SCHEMA_ID}`,
primaryDatasource: toDatasource(
projectId,
dataConnectYaml.location,
dataConnectYaml.schema.datasource,
),
datasources: [
toDatasource(projectId, dataConnectYaml.location, dataConnectYaml.schema.datasource),
],
source: {
files: schemaGQLs,
},
Expand Down
34 changes: 20 additions & 14 deletions src/dataconnect/schemaMigration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,8 +237,11 @@ function diffsEqual(x: Diff[], y: Diff[]): boolean {
}

function setSchemaValidationMode(schema: Schema, schemaValidation: SchemaValidation) {
if (experiments.isEnabled("fdccompatiblemode") && schema.primaryDatasource.postgresql) {
schema.primaryDatasource.postgresql.schemaValidation = schemaValidation;
if (experiments.isEnabled("fdccompatiblemode")) {
const postgresDatasource = schema.datasources.find((d) => d.postgresql);
if (postgresDatasource?.postgresql) {
postgresDatasource.postgresql.schemaValidation = schemaValidation;
}
}
}

Expand All @@ -248,13 +251,12 @@ function getIdentifiers(schema: Schema): {
databaseId: string;
serviceName: string;
} {
const databaseId = schema.primaryDatasource.postgresql?.database;
const postgresDatasource = schema.datasources.find((d) => d.postgresql);
const databaseId = postgresDatasource?.postgresql?.database;
if (!databaseId) {
throw new FirebaseError(
"Schema is missing primaryDatasource.postgresql?.database, cannot migrate",
);
throw new FirebaseError("Service does not have a postgres datasource, cannot migrate");
}
const instanceName = schema.primaryDatasource.postgresql?.cloudSql.instance;
const instanceName = postgresDatasource?.postgresql?.cloudSql.instance;
if (!instanceName) {
throw new FirebaseError(
"tried to migrate schema but instance name was not provided in dataconnect.yaml",
Expand Down Expand Up @@ -509,17 +511,21 @@ async function ensureServiceIsConnectedToCloudSql(
source: {
files: [],
},
primaryDatasource: {
postgresql: {
database: databaseId,
cloudSql: {
instance: instanceId,
datasources: [
{
postgresql: {
database: databaseId,
cloudSql: {
instance: instanceId,
},
},
},
},
],
};
}
const postgresql = currentSchema.primaryDatasource.postgresql;

const postgresDatasource = currentSchema.datasources.find((d) => d.postgresql);
const postgresql = postgresDatasource?.postgresql;
if (postgresql?.cloudSql.instance !== instanceId) {
logLabeledWarning(
"dataconnect",
Expand Down
2 changes: 1 addition & 1 deletion src/dataconnect/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ export interface Service extends BaseResource {
export interface Schema extends BaseResource {
name: string;

primaryDatasource: Datasource;
datasources: Datasource[];
source: Source;
}

Expand Down
31 changes: 16 additions & 15 deletions src/deploy/dataconnect/deploy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,22 +88,23 @@ export default async function (
return !filters || filters?.some((f) => si.dataConnectYaml.serviceId === f.serviceId);
})
.map(async (s) => {
const instanceId = s.schema.primaryDatasource.postgresql?.cloudSql.instance
.split("/")
.pop();
const databaseId = s.schema.primaryDatasource.postgresql?.database;
if (!instanceId || !databaseId) {
return Promise.resolve();
const postgresDatasource = s.schema.datasources.find((d) => d.postgresql);
if (postgresDatasource) {
const instanceId = postgresDatasource.postgresql?.cloudSql.instance.split("/").pop();
const databaseId = postgresDatasource.postgresql?.database;
if (!instanceId || !databaseId) {
return Promise.resolve();
}
const enableGoogleMlIntegration = requiresVector(s.deploymentMetadata);
return provisionCloudSql({
projectId,
locationId: parseServiceName(s.serviceName).location,
instanceId,
databaseId,
enableGoogleMlIntegration,
waitForCreation: true,
});
}
const enableGoogleMlIntegration = requiresVector(s.deploymentMetadata);
return provisionCloudSql({
projectId,
locationId: parseServiceName(s.serviceName).location,
instanceId,
databaseId,
enableGoogleMlIntegration,
waitForCreation: true,
});
}),
);
return;
Expand Down
7 changes: 4 additions & 3 deletions src/init/features/dataconnect/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,16 +261,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);
Copy link
Member

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?

Copy link
Contributor Author

@joehan joehan Sep 18, 2024 •
edited
Loading

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.

if (primaryDatasource?.postgresql?.cloudSql.instance) {
const instanceName = parseCloudSQLInstanceName(
choice.schema.primaryDatasource.postgresql?.cloudSql.instance,
primaryDatasource.postgresql.cloudSql.instance,
);
info.cloudSqlInstanceId = instanceName.instanceId;
}
if (choice.schema.source.files) {
info.schemaGql = choice.schema.source.files;
}
info.cloudSqlDatabase = choice.schema.primaryDatasource.postgresql?.database ?? "";
info.cloudSqlDatabase = primaryDatasource?.postgresql?.database ?? "";
const connectors = await listConnectors(choice.service.name, [
"connectors.name",
"connectors.source.files",
Expand Down
Loading