Cloud SQL create and update polish #7159
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fredzqm
changed the title
rosalyntan
approved these changes
May 10, 2024
src/dataconnect/provisionCloudSql.ts
Outdated
| @@ -27,13 +27,14 @@ export async function provisionCloudSql(args: { | |||
| const existingInstance = await cloudSqlAdminClient.getInstance(projectId, instanceId); | |||
| silent || utils.logLabeledBullet("dataconnect", `Found existing instance ${instanceId}.`); | |||
| connectionName = existingInstance?.connectionName || ""; | |||
| if (!checkInstanceConfig(existingInstance, enableGoogleMlIntegration)) { | |||
| const why = checkInstanceConfig(existingInstance, enableGoogleMlIntegration); | |||
| if (why) { | |||
| // TODO: Return message from checkInstanceConfig to explain exactly what changes are made | |||
There was a problem hiding this comment.
This TODO can be removed now, right?
src/dataconnect/provisionCloudSql.ts
Outdated
| const settings = instance.settings; | ||
| // CloudSQL instances must have public IP enabled to be used with Firebase Data Connect. | ||
| if (!settings.ipConfiguration?.ipv4Enabled) { | ||
| return false; | ||
| return "enable public IP"; |
There was a problem hiding this comment.
Should this add to an array of reasons, so this method returns all the things that need to be updated, rather than just the first one?
There was a problem hiding this comment.
Good idea. Changed it to append to the reason string, so will display it as a list.
src/dataconnect/provisionCloudSql.ts
Outdated
| @@ -95,23 +106,23 @@ export async function provisionCloudSql(args: { | |||
| export function checkInstanceConfig( | |||
There was a problem hiding this comment.
nit: Maybe rename to getUpdateReason and have this return an array of string reasons?
|
Thank you for the review~ Addressed feedbacks~ |
joehan
added a commit
that referenced
this pull request
May 20, 2024
* Cloud SQL create and update polish (#7159) * improve logged message execute SQL commands (#7156) * Error handling and logging when linking CSQL instance (#7158) * Display accurate message for INACCESSIBLE_SCHEMA error (#7157) * handle INACCESSIBLE_SCHEMA case separately * error msg * m * m * m * merge * feedbacks * Jh idx no metadata calls (#7179) * Avoid triggering metadata server calls in idx * Remove outdated service account check * Update Firebase Logo (#7180) * unleash turtles (#7174) * unleash turtles * Update CHANGELOG.md * Flag flip for gen kit (#7175) * Flag flip for gen kit * Update CHANGELOG.md * update icons --------- Co-authored-by: Bryan Kendall <bkend@google.com> Co-authored-by: joehan <joehanley@google.com> * Use client instead of size 1 pool (#7182) * Fix schema validation (#7185) * Fix schema validation * Also depend on redhat.yaml * fix typo --------- Co-authored-by: Fred Zhang <fredzqm@google.com> Co-authored-by: Harold Shen <hlshen@google.com> Co-authored-by: Bryan Kendall <bkend@google.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Explain why update instance is needed
Make create database optional. It fails if CSQL is in the middle of some operations.
Common example is linking an instance just created in cloud console.
Update the create setting a bit to match the default query insights configs.
Otherwise, any instances created via CLI would override it to 0... I consider this a CSQL bug, but see also http://cl/629192474
When customer click edit instance, do nothing and then click save, they see this:
Scenarios Tested
Print update reason:
Sample Commands