fix: delete unused datastore terraform files in agentic_rag projects#797
fix: delete unused datastore terraform files in agentic_rag projects#797eliasecchig merged 1 commit intomainfrom
Conversation
Both vertex_ai_search and vector_search terraform files were being included in generated agentic_rag projects regardless of the selected --datastore flag. The Jinja2 conditionals rendered the file contents as empty but the files themselves remained. Add conditional file deletion entries for all datastore-specific terraform files so only the selected datastore's files are kept. Also fix Makefile template where the Infrastructure Setup section was nested inside an elif for datastore_type instead of being a separate conditional block for cicd_runner.
Summary of ChangesHello @eliasecchig, 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 addresses issues in 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
This pull request effectively addresses two issues in agentic_rag project generation. The Makefile conditional logic is corrected to ensure the Infrastructure Setup section is always available when required, and the new conditional file logic in template.py correctly removes unused datastore-specific Terraform files. The changes are well-implemented. I have one suggestion to refactor the new additions in template.py for improved readability and maintainability.
| # Datastore-specific terraform files (vertex_ai_search vs vertex_ai_vector_search) | ||
| "deployment/terraform/vertex_ai_search.tf": ( | ||
| lambda c: c.get("datastore_type") == "vertex_ai_search" | ||
| ), | ||
| "deployment/terraform/vertex_ai_search_variables.tf": ( | ||
| lambda c: c.get("datastore_type") == "vertex_ai_search" | ||
| ), | ||
| "deployment/terraform/vertex_ai_search_github.tf": ( | ||
| lambda c: c.get("datastore_type") == "vertex_ai_search" | ||
| ), | ||
| "deployment/terraform/dev/vertex_ai_search.tf": ( | ||
| lambda c: c.get("datastore_type") == "vertex_ai_search" | ||
| ), | ||
| "deployment/terraform/dev/vertex_ai_search_variables.tf": ( | ||
| lambda c: c.get("datastore_type") == "vertex_ai_search" | ||
| ), | ||
| "deployment/terraform/vector_search.tf": ( | ||
| lambda c: c.get("datastore_type") == "vertex_ai_vector_search" | ||
| ), | ||
| "deployment/terraform/vector_search_variables.tf": ( | ||
| lambda c: c.get("datastore_type") == "vertex_ai_vector_search" | ||
| ), | ||
| "deployment/terraform/vector_search_github.tf": ( | ||
| lambda c: c.get("datastore_type") == "vertex_ai_vector_search" | ||
| ), | ||
| "deployment/terraform/vector_search_iam.tf": ( | ||
| lambda c: c.get("datastore_type") == "vertex_ai_vector_search" | ||
| ), | ||
| "deployment/terraform/vector_search_service_accounts.tf": ( | ||
| lambda c: c.get("datastore_type") == "vertex_ai_vector_search" | ||
| ), | ||
| "deployment/terraform/dev/vector_search.tf": ( | ||
| lambda c: c.get("datastore_type") == "vertex_ai_vector_search" | ||
| ), | ||
| "deployment/terraform/dev/vector_search_variables.tf": ( | ||
| lambda c: c.get("datastore_type") == "vertex_ai_vector_search" | ||
| ), | ||
| "deployment/terraform/dev/vector_search_iam.tf": ( | ||
| lambda c: c.get("datastore_type") == "vertex_ai_vector_search" | ||
| ), |
There was a problem hiding this comment.
To improve readability and reduce code duplication, you can use dict.fromkeys combined with dictionary unpacking (**) to assign the same conditional lambda to multiple files. This makes the code more concise and easier to maintain if the condition needs to be updated in the future.
# Datastore-specific terraform files (vertex_ai_search vs vertex_ai_vector_search)
**dict.fromkeys(
(
"deployment/terraform/vertex_ai_search.tf",
"deployment/terraform/vertex_ai_search_variables.tf",
"deployment/terraform/vertex_ai_search_github.tf",
"deployment/terraform/dev/vertex_ai_search.tf",
"deployment/terraform/dev/vertex_ai_search_variables.tf",
),
lambda c: c.get("datastore_type") == "vertex_ai_search",
),
**dict.fromkeys(
(
"deployment/terraform/vector_search.tf",
"deployment/terraform/vector_search_variables.tf",
"deployment/terraform/vector_search_github.tf",
"deployment/terraform/vector_search_iam.tf",
"deployment/terraform/vector_search_service_accounts.tf",
"deployment/terraform/dev/vector_search.tf",
"deployment/terraform/dev/vector_search_variables.tf",
"deployment/terraform/dev/vector_search_iam.tf",
),
lambda c: c.get("datastore_type") == "vertex_ai_vector_search",
),1 participant
Summary
--datastoreflagProblem
Generated
agentic_ragprojects included bothvertex_ai_search_*andvector_search_*terraform files regardless of which--datastorewas selected. The Jinja2 conditionals rendered file contents as empty, but the empty files themselves remained in the project, causing terraform to pick them up and deploy conflicting infrastructure.Additionally, the Makefile template had the Infrastructure Setup section nested inside an
eliffordatastore_type == "vertex_ai_search", making it unavailable forvertex_ai_vector_searchprojects.Changes
agent_starter_pack/cli/utils/template.py: Add 13 entries toCONDITIONAL_FILESmapping each datastore-specific terraform file (both top-level anddev/) to its datastore type condition. Files for the non-selected datastore are deleted during project generation.agent_starter_pack/base_templates/python/Makefile: Splitelif cookiecutter.cicd_runner != 'skip'into a separateifblock so the Infrastructure Setup section is independent of the datastore conditional.