fix: use cleaned relative path instead of absolute path for local module hash#5280
Conversation
Summary of ChangesHello @mtibben, 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 delivers significant improvements in development consistency and introduces new infrastructure deployment capabilities. It rectifies a core issue with local module hashing that caused environment-specific Terraform diffs and updates the baseline Terraform version. Furthermore, it expands the toolkit's offerings with new GKE consumption options for advanced TPU hardware and a dedicated blueprint for A4X Max bare metal GPUs. The changes also include enhancements to logging for better traceability and a new validation mechanism for more robust blueprint configurations. Highlights
Changelog
Ignored Files
Activity
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
|
There was a problem hiding this comment.
Code Review
This pull request fixes a bug where the hash generated for local Terraform modules was not deterministic across different developer machines, causing spurious diffs. The change replaces the use of filepath.Abs with filepath.Clean to generate the hash from a normalized, machine-independent path string. The implementation is correct and effectively solves the issue. The associated test changes are also good, making the tests deterministic and adding a new test case for equivalent paths. I have one minor suggestion in the test file to improve consistency and robustness.
4a230fc to
58ec1a1
Compare
|
@SwarnaBharathiMantena @samskillman let me know if you have any feedback |
|
/gcbrun |
SwarnaBharathiMantena
left a comment
There was a problem hiding this comment.
Thanks for submitting a PR. LGTM.
(Apologies on the delayed review)
58ec1a1 to
f3b1a66
Compare
…ule hash Replace filepath.Abs with filepath.Clean when computing the short hash for local module deployment source directories. This ensures the hash is deterministic across machines regardless of where the repository is checked out.
f3b1a66 to
178fbfc
Compare
|
/gcbrun |
3db9949
into
GoogleCloudPlatform:develop
When
gcluster createcomputes the deployment source directory name for local terraform modules, it generates a short hash to disambiguate modules with the same basename. Previously this hash was based onfilepath.Abs(mod.Source), which resolves relative to the current working directory. Since the absolute path includes the user's home directory and checkout location, different developers running the same blueprint get different hashes, producing spurious terraform diffs.This changes the hash input from the absolute path to
filepath.Clean(mod.Source), which normalises the path string (resolves.., removes redundant separators/dots) without resolving against the working directory. The cleaned path is the same on every machine because it comes directly from the shared blueprint YAML.Alternatives considered:
Hash relative to the blueprint file's directory — resolve
mod.Sourceagainst the blueprint dir, then make it relative to the blueprint dir again. For relative paths this produces the same result asfilepath.Clean. However it breaks at deploy time: the expanded blueprint is re-loaded from the artifacts directory (.ghpc/expanded_blueprint.yaml), so the blueprint dir differs between creation and deploy, and the hash no longer matches.Hash relative to the git repository root — resolve
mod.Sourceto an absolute path and then make it relative to the git root. This strips the machine-specific prefix and works at creation time, but has the same deploy-time problem:mod.Sourceis still the raw string from the YAML, and resolving it against a different context (the artifacts directory) produces a different relative-to-git-root path.Hash the directory contents — truly content-addressed, but expensive (must walk the full directory tree), and the hash would change on every content edit, causing spurious terraform diffs during development.
Both resolution-based alternatives fail because
mod.Sourceis stored as-is in the expanded blueprint and is not rewritten when the blueprint is saved to the artifacts directory. Any approach that resolves the path against a contextual anchor (CWD, blueprint dir, git root) produces inconsistent results between creation and deploy.filepath.Cleanavoids this by operating purely on the path string.