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.

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

Cleaning up logging in dataconnect:sql:setup #8380

Merged
merged 5 commits into from
Apr 1, 2025
Merged

Conversation

joehan
Copy link
Contributor

@joehan joehan commented Mar 27, 2025

Description

A couple logging clean ups:

  • Respect the silent flag as much as possible in permissionsSetup
  • Move Google Analytics logs to debug level
  • Move some default case logs for SQL set up to debug level
  • Rename file to use camelcase.

Scenarios Tested

Screenshot 2025-03-27 at 2 46 20 PM

@joehan joehan requested a review from tammam-g March 27, 2025 21:48
@@ -343,21 +340,23 @@ export async function setupBrownfieldAsGreenfield(
const schema = schemaInfo.name;

const firebaseOwnerRole = firebaseowner(databaseId, schema);
const uniqueTablesOwners = filterTableOwners(schemaInfo, databaseId);
const nonFirebasetablesOwners = [...new Set(schemaInfo.tables.map((t) => t.owner))].filter(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is intentional or not but this is a regression, I think we should keep filterTableOwners or make sure to be also filter CLOUDSQL_SUPER_USER.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely, this was just a screwed up merge. Fixing now

@@ -394,8 +393,8 @@ export async function brownfieldSqlSetup(
) {
const schema = schemaInfo.name;

// Step 1: Grant firebasesuperuser access to the original owner.
const uniqueTablesOwners = filterTableOwners(schemaInfo, databaseId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

@joehan joehan requested a review from tammam-g March 28, 2025 21:00
@joehan joehan enabled auto-merge (squash) April 1, 2025 20:10
@joehan joehan merged commit 14f50bb into master Apr 1, 2025
48 of 49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants