[DCP] Use the image tag instead of latest in TF deployments#19
[DCP] Use the image tag instead of latest in TF deployments#19gmechali wants to merge 6 commits intodatacommonsorg:mainfrom
Conversation
…s a new script to fetch the latest image tag and upadte the tfvars.
Summary of ChangesHello, 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 significantly enhances the deployment and management of Data Commons Platform (DCP) infrastructure using Terraform. The core improvement addresses a common issue with container image deployments where using the "latest" tag prevents infrastructure-as-code tools like Terraform from detecting and applying updates. By introducing explicit image tag management and an automation script, users can now reliably update their DCP images. Furthermore, the PR modularizes the infrastructure into distinct DCP and legacy CDC stacks, improving organization, flexibility, and maintainability for future deployments. Highlights
Changelog
Using Gemini Code AssistThe 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 by creating a comment using either
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 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces a new Terraform module for deploying the Data Commons Platform (DCP) alongside an existing module for the Custom Data Commons (CDC) stack, allowing users to enable either or both. Key changes include adding new Terraform and configuration files to manage GCP resources like Cloud Run services, Spanner instances, MySQL, and Redis, along with updates to .gitignore and setup scripts. Review comments highlight several improvement opportunities: the .terraform.lock.hcl file should be committed for consistent provider versions, a redundant google_project_service resource in the CDC module needs removal to prevent conflicts, and the FORCE_RESTART environment variable using timestamp() in the CDC module should be removed to avoid unnecessary Cloud Run redeployments. Additionally, the logic for constructing Spanner instance and database names is duplicated across multiple DCP module files and should be refactored into modules/dcp/locals.tf for better maintainability.
| terraform.rc | ||
| tfplan | ||
| infra/dcp/backend.tf | ||
| .terraform.lock.hcl |
There was a problem hiding this comment.
The .terraform.lock.hcl file should be committed to version control, not ignored. According to the official Terraform documentation, this file locks the provider versions used for your configuration. Committing it ensures that all team members and CI/CD systems use the exact same provider versions, which prevents inconsistencies and potential "works on my machine" issues.
| resource "google_project_service" "required_apis" { | ||
| for_each = toset([ | ||
| "run.googleapis.com", | ||
| "sqladmin.googleapis.com", | ||
| "compute.googleapis.com", | ||
| "redis.googleapis.com", | ||
| "secretmanager.googleapis.com", | ||
| "vpcaccess.googleapis.com", | ||
| "artifactregistry.googleapis.com", | ||
| "iam.googleapis.com" | ||
| ]) | ||
| project = var.project_id | ||
| service = each.value | ||
| disable_on_destroy = false | ||
| } |
There was a problem hiding this comment.
This google_project_service resource is redundant. The root module (infra/dcp/main.tf) already enables all the necessary APIs for both the DCP and CDC stacks. This duplication will cause conflicts during terraform apply because both resources will attempt to manage the same project services. Please remove this entire resource block.
| { | ||
| name = "FORCE_RESTART" | ||
| value = "${timestamp()}" | ||
| }, |
There was a problem hiding this comment.
The FORCE_RESTART environment variable is set using timestamp(). This will force the Cloud Run service to redeploy on every terraform apply, even when no configuration has changed. This can lead to unnecessary service restarts and downtime. Since the goal is to move towards immutable image tags, this forced restart mechanism might no longer be necessary or desirable. Consider removing it to avoid unexpected deployments.
1 participant
FYI - this PR is a child of #15
If you use the image tag "latest" , terraform won't know whether the latest image has changed and will not re-deploy the new image. This makes it more annoying for a customer who may want to update to the latest DCP image.
With the current changes, the dcp image tag is a variable passed in which defaults to latest, but if they run ./update_image_tag.sh, it will modify their terraform.tfvars file to apply the latest image's tag.
Therefore, they will be able to run terraform plan to view what would get re-deployed