feat: implement lean deployment modules by selective copying#5482
Conversation
4786b7f to
9404af1
Compare
|
/gemini review |
9404af1 to
c2f8045
Compare
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 introduces a significant optimization to how embedded modules are handled during deployment. The system now intelligently resolves and copies only those embedded modules that are directly referenced or are transitive dependencies of referenced modules. This change aims to create leaner and more efficient deployment directories by avoiding the copying of unneeded modules. Highlights
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. Footnotes
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
c2f8045 to
c13330e
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a 'lean deployment' strategy for embedded modules by resolving and copying only the necessary transitive local dependencies instead of the entire modules directory. It introduces GetLocalDependencies and ResolveDependencies in pkg/modulereader and refactors pkg/modulewriter to utilize these functions. Feedback suggests refactoring duplicated module loading logic in hcl_utils.go into a shared helper and suggests documenting that transitive dependency resolution is currently limited to embedded modules.
There was a problem hiding this comment.
Code Review
This pull request implements transitive local dependency resolution for modules, ensuring that all required local sub-modules are correctly identified and copied during deployment generation. Key changes include the addition of GetLocalDependencies and ResolveDependencies functions, refactoring the modulewriter to utilize these functions, and updating path manipulations to use filepath for better cross-platform compatibility. Comprehensive unit and integration tests have been added to validate the new functionality. Feedback suggests refactoring duplicated module loading logic in hcl_utils.go into a shared helper function to improve maintainability and prevent potential nil-pointer panics.
c13330e to
78fc42f
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements transitive dependency resolution for Terraform modules to optimize the deployment process. By introducing GetLocalDependencies and ResolveDependencies, the toolkit now identifies and copies only the specific embedded modules required by a blueprint, rather than copying the entire modules library. The changes include refactoring modulewriter to utilize this new logic, standardizing on filepath for local path operations, and adding comprehensive unit and integration tests. Review feedback recommends normalizing input module paths to ensure robust deduplication during dependency resolution and removing a redundant path cleaning operation.
78fc42f to
b784b94
Compare
|
/gemini review |
b784b94 to
518c544
Compare
There was a problem hiding this comment.
Code Review
This pull request implements selective copying of embedded modules by introducing a transitive dependency resolver. It refactors module loading to trace local dependencies and updates the deployment writer to copy only referenced embedded modules. Feedback highlights that dependency resolution is currently limited to embedded modules, suggesting this behavior be documented to clarify support for user-provided local modules.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a dependency-aware module copying process to ensure only necessary embedded modules are included in deployments. Key changes include the addition of a dependency resolver in pkg/modulereader, refactoring pkg/modulewriter to utilize transitive dependency information, and the introduction of comprehensive unit and integration tests. Review feedback highlights opportunities to clarify path documentation in GetLocalDependencies, optimize slice allocations for performance in copyGroupSources, and eliminate redundant copies of Packer modules to further streamline the deployment output.
518c544 to
87a8973
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a dependency resolution mechanism for embedded modules to optimize the deployment generation process. Instead of copying the entire embedded modules directory, the toolkit now identifies and copies only the specific modules required by a blueprint and their transitive local dependencies. This is achieved through new functions GetLocalDependencies and ResolveDependencies in the modulereader package. Additionally, the PR refactors path handling across several files to prefer filepath over path for OS-level operations and adds comprehensive unit and integration tests. Feedback is provided regarding cross-platform path normalization on Windows and idiomatic usage of filepath.Join.
87a8973 to
d28f9a2
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request optimizes the deployment process by introducing a dependency resolver that identifies and copies only the required transitive local dependencies for embedded modules. It also refactors path handling to use filepath for better cross-platform compatibility and improves error reporting during module loading. The review feedback suggests including the module source path in error messages to further improve debuggability.
d28f9a2 to
c8223e9
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a shared directory mechanism for embedded modules, centralizing them within a _modules directory to handle transitive dependencies more effectively. Key updates include reserving specific group names, implementing a dependency resolver, and refactoring the module writer to support this new structure. Feedback suggests enhancing the robustness of module copying by ensuring parent directories are created and recommends using post-order traversal in the dependency resolver to ensure dependencies are listed before the modules that rely on them.
|
Thanks for the review! Here is my response to the feedback points:
I will implement the fix for point 1 shortly. |
8da6ecc to
5b1ac6d
Compare
|
/gemini review |
|
/gcbrun |
There was a problem hiding this comment.
Code Review
This pull request introduces a shared directory mechanism (_modules) for embedded modules to optimize the deployment structure and avoid redundant copies. It implements transitive dependency resolution for embedded modules and updates path handling to use filepath for improved cross-platform compatibility. Feedback suggests optimizing the module copying process by using a package-level map to track created directories, thereby reducing redundant os.MkdirAll calls.
5b1ac6d to
791ea0b
Compare
|
/gcbrun |
1 similar comment
|
/gcbrun |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request restructures the deployment generation process to use a shared _modules directory for embedded modules, supported by new logic for resolving transitive local dependencies. This ensures that nested module references are correctly handled in the output. A review comment suggested an optimization to filter for Terraform modules during dependency collection to avoid redundant processing of Packer modules, which are managed separately.
* Added dependency resolution in pkg/modulereader using tfconfig * Refactored modulewriter to copy only referenced embedded modules * Added unit tests and an integration test verifying terraform init
Fix cross-module file references by moving embedded modules to a shared directory at the deployment root instead of per-group directories. Add validation to prevent deployment groups from using reserved names (.ghpc, _modules). Use a constant for the directory name to avoid hardcoding. TAG=agy CONV=ba441f3b-312a-4a96-a27a-dc531d793750
Fixes path failures in GCluster ML A3 Slurm blueprints. The path to long-prolog-slurm.conf.tpl inside the controller module etc/ directory was hardcoded as modules/embedded/.... Since all embedded modules are now stored under a single shared root directory _modules/embedded/ to fix cross-module references, these hardcoded paths in the blueprints must be updated to points to the new shared directory relative to the group directory (../_modules/embedded/...). TAG=agy CONV=ba441f3b-312a-4a96-a27a-dc531d793750
Packer modules are managed and copied separately, and they do not share HCL dependencies like Terraform modules. Filtering them out from the dependency collection loop prevents redundant processing and parsing attempts by the HCL resolver. TAG=agy CONV=ba441f3b-312a-4a96-a27a-dc531d793750
222678a to
320422f
Compare
|
/gcbrun |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a shared directory structure (_modules) for embedded modules within deployments, enabling the resolution and inclusion of transitive local dependencies. Key changes include the implementation of a dependency resolver using depth-first search in pkg/modulereader, updates to the module writer to centralize embedded modules, and the reservation of the _modules name in group validation. Additionally, several files were refactored to use filepath instead of path for better cross-platform compatibility. Feedback is provided to ensure that parent directories are explicitly created when copying non-embedded modules to maintain consistency and prevent potential filesystem errors.
Explicitly create parent directories using os.MkdirAll before copying non-embedded modules. This addresses code review feedback to ensure robustness and consistency with the shared embedded modules copying logic, preventing potential filesystem errors if the underlying loaders assume parent folders exist. TAG=agy CONV=ba441f3b-312a-4a96-a27a-dc531d793750
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request optimizes deployments by introducing a shared "_modules" directory for embedded modules, replacing the previous behavior of copying the entire modules library into each group. It includes a new dependency resolver that performs a depth-first search to identify and copy only the transitive local dependencies required by the blueprint. Additionally, the group name validation was updated to reserve ".ghpc" and "_modules". A suggestion was made to improve the consistency of string comparisons in the group name validation logic.
|
In principle, babysit ran all the regression tests in the past 2 week. |
Submission Checklist
NOTE: Community submissions can take up to 2 weeks to be reviewed.
Please take the following actions before submitting this pull request.