-
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
"Connect to Postgres" using ConfigureEmulator API #7262
Conversation
hlshen
commented
Jun 4, 2024
•
edited
Loading
edited
- Add UI for Connect to Postgres, and displaying postgres connection after
- Local endpoint is a class function in EmulatorController
- Implement DataConnectEmulatorClient, and ConfigureEmulator api
} | ||
|
||
return ( | ||
"http://" + dataConnectEmulator.host + ":" + dataConnectEmulator.port |
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.
Please handle IPv6 quoting similar to
firebase-tools/src/emulator/registry.ts
Lines 180 to 181 in 1633d69
if (info.host.includes(":")) { | |
url.hostname = `[${info.host}]`; // IPv6 addresses need to be quoted. |
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.
Mostly looking good but please address comments inline
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.
General approach looks good, but i think this won't behave as expected rn.
* Autoconnect to postgres when not in VSCE * Format * Return values
Merged my PR onto yours to autoconnect when not in vscode |
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.
LGTM after formatting
}); | ||
|
||
return connectionString; | ||
} | ||
|
||
private async connectToPostgres() { | ||
const rc = firebaseRC.value?.tryReadValue; | ||
const newConnectionString = await this.promptConnectionString( | ||
rc?.getDataconnect()?.postgres.localConnectionString || | ||
"postgres://user:password@localhost:5432/dbname", |
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.
Should we use switch the default string to point to the 'postgres' DB?
"postgres://user:password@localhost:5432/dbname", | |
"postgres://user:password@localhost:5432/postgres", |
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.
yes
src/emulator/dataconnectEmulator.ts
Outdated
@@ -2,13 +2,17 @@ import * as childProcess from "child_process"; | |||
|
|||
import { dataConnectLocalConnString } from "../api"; | |||
import { Constants } from "./constants"; | |||
import { getPID, start, stop, downloadIfNecessary } from "./downloadableEmulators"; | |||
import { getPID, start, stop, downloadIfNecessary |
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.
Looks like we just need to run 'npm run format'