Conversation
![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: 4ea1c80a82
ℹ️ 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 db_driver == "cloudsql": | ||
| db_name = os.getenv("CLOUD_SQL_DATABASE", "") | ||
| instance_name = os.getenv("CLOUD_SQL_INSTANCE_NAME", "") | ||
| iam_auth = os.getenv("CLOUD_SQL_IAM_AUTH", "") |
There was a problem hiding this comment.
When DB_DRIVER=cloudsql, this branch only logs Cloud SQL settings and no longer enforces the prior guard that aborted on test databases. As a result, a run with CLOUD_SQL_DATABASE=ocotilloapi_test or nmsamplelocations_test now continues into transfer_all and can mutate/drop data that was previously protected by a fail-fast check. Add the same test-database validation in the Cloud SQL branch before the transfer starts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Updates the transfer runner to better support managed runtimes by defaulting into Cloud SQL configuration when Cloud SQL env vars are present, and by logging Cloud SQL connection configuration instead of local Postgres settings.
Changes:
- Default
DB_DRIVERtocloudsqlwhenCLOUD_SQL_INSTANCE_NAMEis set butDB_DRIVERis missing. - Default
CLOUD_SQL_IAM_AUTHto"true"when using Cloud SQL (unless explicitly set). - Update startup logging in
main()to display Cloud SQL connection details whenDB_DRIVER=cloudsql.
| message( | ||
| "Database Configuration: " | ||
| f"driver=cloudsql instance={instance_name} db={db_name} iam_auth={iam_auth}" | ||
| ) |
There was a problem hiding this comment.
The safety guard that blocks running transfers against known test DBs only runs in the non-Cloud SQL branch. When DB_DRIVER=cloudsql, db_name can still point at *_test and the script will proceed. Consider applying the same check to CLOUD_SQL_DATABASE (and failing fast for the same test DB names / non-dev DBs) so Cloud SQL executions get the same protection.
| ) | |
| ) | |
| # Double-check we're using the development database for Cloud SQL as well | |
| if db_name != "ocotilloapi_dev": | |
| message(f"WARNING: Using database '{db_name}' instead of 'ocotilloapi_dev'") | |
| if db_name in ("ocotilloapi_test", "nmsamplelocations_test"): | |
| raise ValueError( | |
| "ERROR: Cannot run transfer on test database! " | |
| "Set CLOUD_SQL_DATABASE=ocotilloapi_dev in environment" | |
| ) |
| # In managed runtime environments, DB_DRIVER is occasionally omitted while | ||
| # CLOUD_SQL_* vars are present. Default to cloudsql in that case to avoid | ||
| # silently falling back to localhost/postgres settings. | ||
| if ( | ||
| not (os.getenv("DB_DRIVER") or "").strip() | ||
| and (os.getenv("CLOUD_SQL_INSTANCE_NAME") or "").strip() | ||
| ): |
There was a problem hiding this comment.
Comment/condition mismatch: the comment says "CLOUD_SQL_* vars are present", but the fallback to cloudsql only checks CLOUD_SQL_INSTANCE_NAME. Either update the comment to match the actual condition, or expand the condition to check the required Cloud SQL vars you expect to be present (e.g., database/user) before forcing DB_DRIVER=cloudsql.
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?