Conversation
…g password handling
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2f4b86ff2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not use_iam_auth: | ||
| raise RuntimeError( | ||
| "CLOUD_SQL_IAM_AUTH must be true when DB_DRIVER=cloudsql." | ||
| ) |
There was a problem hiding this comment.
This branch now throws whenever CLOUD_SQL_IAM_AUTH=false, which removes previously supported Cloud SQL password authentication and causes runtime failures for any deployment that explicitly disables IAM auth. The prior implementation used CLOUD_SQL_PASSWORD in this path, and repository logic still treats IAM as a default that can be overridden (for example, transfers/transfer.py says it should default to IAM unless explicitly disabled), so this is a behavioral regression that can break existing cloudsql environments on first DB use.
Useful? React with 👍 / 👎.
| if not use_iam_auth: | ||
| raise RuntimeError( | ||
| "CLOUD_SQL_IAM_AUTH must be true when DB_DRIVER=cloudsql." | ||
| ) |
There was a problem hiding this comment.
The migration connector now hard-fails when CLOUD_SQL_IAM_AUTH is false, so alembic upgrade cannot run in Cloud SQL setups that use DB password auth instead of IAM tokens. This also leaves inconsistent behavior in the same file because build_database_url() still has a non-IAM/password branch, indicating non-IAM mode is expected to exist, but online migrations now crash before connecting.
Useful? React with 👍 / 👎.
| if not use_iam_auth: | ||
| raise RuntimeError( |
There was a problem hiding this comment.
The get_bool_env function can return a string value if the environment variable is set to something other than recognized boolean values ("true", "1", "yes", "false", "0", "no"). This could lead to unexpected behavior in the if not use_iam_auth: check. Consider adding validation to ensure use_iam_auth is actually a boolean, or handle the case where it might be an unexpected string value.
| if not use_iam_auth: | ||
| raise RuntimeError( |
There was a problem hiding this comment.
The get_bool_env function can return a string value if the environment variable is set to something other than recognized boolean values ("true", "1", "yes", "false", "0", "no"). This could lead to unexpected behavior in the if not use_iam_auth: check. Consider adding validation to ensure use_iam_auth is actually a boolean, or handle the case where it might be an unexpected string value.
| @@ -80,10 +79,11 @@ def asyncify_connection(): | |||
| "enable_iam_auth": use_iam_auth, | |||
There was a problem hiding this comment.
Since the code now enforces that IAM auth must be enabled (raising an error if it's not), the enable_iam_auth parameter in connect_kwargs should be hardcoded to True instead of using the use_iam_auth variable. This makes the intent clearer and avoids passing a value that's always True at this point in the code.
| "enable_iam_auth": use_iam_auth, | |
| "enable_iam_auth": True, |
| @@ -118,10 +117,11 @@ def getconn(): | |||
| "ip_type": ip_type, | |||
| "enable_iam_auth": use_iam_auth, | |||
There was a problem hiding this comment.
Since the code now enforces that IAM auth must be enabled (raising an error if it's not), the enable_iam_auth parameter in connect_kwargs should be hardcoded to True instead of using the use_iam_auth variable. This makes the intent clearer and avoids passing a value that's always True at this point in the code.
| @@ -147,10 +146,11 @@ def getconn(): | |||
| "ip_type": ip_type, | |||
| "enable_iam_auth": use_iam_auth, | |||
There was a problem hiding this comment.
Since the code now enforces that IAM auth must be enabled (raising an error if it's not), the enable_iam_auth parameter in connect_kwargs should be hardcoded to True instead of using the use_iam_auth variable. This makes the intent clearer and avoids passing a value that's always True at this point in the code.
| "enable_iam_auth": use_iam_auth, | |
| "enable_iam_auth": True, |
Why
This PR addresses the following problem / context:
How
Implementation summary - the following was changed / added / removed:
Notes
Any special considerations, workarounds, or follow-up work to note?