Skip to content

Improve fdc permissions setup #8339

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

Merged
merged 14 commits into from
Mar 20, 2025
Merged

Improve fdc permissions setup #8339

merged 14 commits into from
Mar 20, 2025

Conversation

tammam-g
Copy link
Contributor

@tammam-g tammam-g commented Mar 19, 2025

This PR improves the FDC schema setup:
1- Make sure IAM user is upserted before sql:diff
2- Refactor brownfield setups to revoke owners after setup to avoid getting left with IAM roles.
3- Use transactions for schema setup commands to avoid problems when setup fails in intermediate state (e.g if we assign IAM role to firebasesuperuser then firebasesuperuser becomes IAM role and it becomes not possible to login using that role).
4- If schema is setup as brownfield then schema migration will fail without prompting the user to setup. If users decide to change their setup then they should run the setup command explicitly (this change doesn't affect fresh database, they are still automatically setup).
5- Improve messaging (tell users to turn on compate mode)

Scenarios tested:

  • Completely fresh database is automatically setup as greenfield without any prompting
  • Brownfield database with IAM tables setup as greenfield
  • Ctrl+c half way through brownfield setup and confirming IAM role wasn't assigned to firebasesuperuser (transaction wasn't commited)

@tammam-g tammam-g requested a review from joehan March 19, 2025 21:33
Copy link
Contributor

@joehan joehan left a comment

Choose a reason for hiding this comment

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

Some ntis and suggestions, but mostly LGTM

logger.info(clc.green("Database setup complete."));
logger.info(
clc.yellow(
"IMPORTANT: please uncomment 'schemaValidation: \"COMPATIBLE\"' in your dataconnect.yaml file to avoid dropping any existing tables by mistake.",
Copy link
Contributor

Choose a reason for hiding this comment

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

(can be in a follow up) Should we just uncomment it by default in this case? Seems like a foot gun otherwise

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you aware of an easy way to do the uncommenting?

@joehan
Copy link
Contributor

joehan commented Mar 20, 2025

Could you also add some Scenarios Tested in the PR description?

@tammam-g tammam-g enabled auto-merge (squash) March 20, 2025 18:56
@tammam-g tammam-g disabled auto-merge March 20, 2025 18:57
@tammam-g tammam-g merged commit 57eed28 into master Mar 20, 2025
52 of 55 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