Skip to content

Conversation

@fredzqm
Copy link
Contributor

@fredzqm fredzqm commented Aug 20, 2025

Description

Bug-bash dataconnect:sql:* command extensively. Found and fixed many bugs:

  • P0: First sql:migrate & deploy fails due to SQL owner permission is missing. Anything schema created in console run into this.
  • P1: When users say Abort changes in SQL prompt, we should actually abort 😄 as opposed to skip them.
  • P1: When schema is invalid, sql:diff / sql:migrate fails with an opaque un-informative error message.
  • P2: Make SQL migration choices clear. Show "Execute all (including destructive)" vs "Execute safe only" when applicable.
  • P2: Make SQL migration message brief and clear. Use instanceId instead of the long instanceName.
  • P2: sql:diff was diffing twice (COMPATIBLE then STRICT). This PR changed it to diff in STRICT first, and short-circuit if there is no diff.

Scenarios Tested

SQL permission bug

Create a schema in console or the new CLI local flow.

Screenshot 2025-08-20 at 5 39 59 PM

Edit the schema and run sql:migrate or deploy.
Previously it would get rejected due to lack of SQL owner permission. Now it would automatically grant it.

Another way to reproduce this is. Person A) set up FDC and then person B) try to update and migrate it.

Screenshot 2025-08-20 at 5 41 06 PM

Other misc cases

Screenshot 2025-08-20 at 5 05 06 PM Screenshot 2025-08-20 at 5 09 32 PM Screenshot 2025-08-20 at 5 11 09 PM Screenshot 2025-08-20 at 5 11 49 PM

Sample Commands

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Summary of Changes

Hello @fredzqm, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request focuses on enhancing the sql:migrate command within Firebase Data Connect. The primary goal is to resolve permission-related issues and improve the overall user experience by providing clearer messages and more robust handling of schema migrations. The changes streamline the process of setting up and migrating Cloud SQL databases, particularly concerning user roles and the application of schema changes under different validation modes.

Highlights

  • Enhanced Permission Management: The sql:migrate command now includes improved permission handling. If a user is a Cloud SQL Admin but lacks the necessary firebaseowner SQL role for migration, the system will now attempt to automatically grant this role, reducing friction and permission-related errors.
  • Refactored Schema Migration Logic: The logic for schema diffing and migration has been significantly refactored. This includes clearer messaging for users regarding 'STRICT' versus 'COMPATIBLE' schema validation modes, and more consistent output during the process of generating and applying SQL migrations.
  • Centralized SQL Role Granting: The process of granting SQL roles to users has been centralized into a new, reusable grantRoleTo function. This improves code modularity and maintainability by consolidating permission-related operations.
  • Improved User Prompting and Command Behavior: User prompts for schema migrations have been refined to provide more explicit choices, especially when dealing with optional or destructive changes. The behavior for non-interactive and force modes has also been made more robust and predictable.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces significant improvements to the sql:migrate command flow in Firebase Data Connect. It fixes permission-related bugs, enhances user-facing messages for clarity, and refactors complex logic for better maintainability. Key changes include automatically granting the firebaseowner role to Cloud SQL admins, clearer migration prompts, and more descriptive logging. The introduction of a reusable grantRoleTo function is a good architectural improvement.

I have found one bug where an incorrect user identifier is passed when attempting to grant roles, which would cause issues for service accounts. I've also pointed out a leftover TODO comment that should be removed. Overall, these are great changes that will improve the developer experience.

@fredzqm fredzqm requested review from rosalyntan and tammam-g August 21, 2025 00:03
Copy link
Contributor

@tammam-g tammam-g left a comment

Choose a reason for hiding this comment

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

Thanks for fixing these bugs, it's tricky. Glancing over this I don't see any issues it would cause. I think brownfield setup should be not affected.

@fredzqm fredzqm enabled auto-merge (squash) August 21, 2025 03:03
@fredzqm
Copy link
Contributor Author

fredzqm commented Aug 21, 2025

Looks like I missed this week's release. It's alright. Next Wednesday then

@fredzqm fredzqm merged commit 76f9f37 into master Aug 21, 2025
48 checks passed
@fredzqm fredzqm deleted the fz/fdc-deploy branch August 21, 2025 05:53
@github-project-automation github-project-automation bot moved this from Approved [PR] to Done in [Cloud] Extensions + Functions Aug 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants