- Notifications
You must be signed in to change notification settings - Fork 1.1k
[FDC] Fix sql:migrate permission bugs and improve messages #9018
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
Conversation
Co-authored-by: Joe Hanley <joehanley@google.com>
Co-authored-by: Joe Hanley <joehanley@google.com>
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.
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:migratecommand now includes improved permission handling. If a user is a Cloud SQL Admin but lacks the necessaryfirebaseownerSQL 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
grantRoleTofunction. 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
-
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. ↩
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.
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.
tammam-g left a comment
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.
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.
| Looks like I missed this week's release. It's alright. Next Wednesday then |
Description
Bug-bash
dataconnect:sql:*command extensively. Found and fixed many bugs:sql:migrate&deployfails due to SQL owner permission is missing. Anything schema created in console run into this.Abort changesin SQL prompt, we should actually abort 😄 as opposed to skip them.sql:diff/sql:migratefails with an opaque un-informative error message.instanceIdinstead of the longinstanceName.sql:diffwas diffing twice (COMPATIBLE then STRICT). This PR changed it to diff inSTRICTfirst, and short-circuit if there is no diff.Scenarios Tested
SQL permission bug
Create a schema in console or the new CLI local flow.
Edit the schema and run
sql:migrateordeploy.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.
Other misc cases
Sample Commands