Copy lock file only when needed #2823
Conversation
|
|
@denis256 I had some trailing whitespaces. Fixed in the latest commit by running terragrunt/.circleci/config.yml Line 105 in e5081e1 |
util/file.go
Outdated
| return errors.WithStackTrace(destReadErr) | ||
| } | ||
|
|
||
| if string(sourceContents) == string(destinationContents) { |
There was a problem hiding this comment.
util/file.go:613:5: stringXbytes: suggestion: bytes.Equal(sourceContents, destinationContents) (gocritic)
if string(sourceContents) == string(destinationContents) {
|
Failed integration tests: |
|
Will be also helpful to add an integration test which will track that the lock copy works as expected. |
ben-z
force-pushed
the
support-readonly-fs
branch
from
9fa1844 to
889aca5
Compare
I added some unit tests. Integration tests seem a lot more involved and I'm not super familiar with Go. Are unit tests sufficient? |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for submitting this pull request. |
cc @denis256 |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for submitting this pull request. |
📝 WalkthroughWalkthroughThis pull request refines the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant CopyLockFile
participant FileSystem
participant Logger
Caller->>CopyLockFile: Call CopyLockFile(src, dest)
CopyLockFile->>FileSystem: Compute absolute paths for src & dest
alt Paths are identical
CopyLockFile->>Logger: Log identical path message
CopyLockFile-->>Caller: Return early
else
CopyLockFile->>FileSystem: Check source file existence
alt Source file not found
CopyLockFile->>Logger: Log missing source
CopyLockFile-->>Caller: Return early
else
CopyLockFile->>FileSystem: Read source file
CopyLockFile->>FileSystem: Check destination file existence
alt Destination file not found
CopyLockFile->>Logger: Log action to copy file
CopyLockFile->>FileSystem: Copy file preserving permissions
else
CopyLockFile->>FileSystem: Read destination file
alt Contents are identical
CopyLockFile->>Logger: Log no copy needed
CopyLockFile-->>Caller: Return early
else
CopyLockFile->>FileSystem: Copy file preserving permissions
end
end
end
end
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
util/file_test.go (1)
5-6: Consider usingospackage instead ofioutil.
Althoughioutilstill works, it’s encouraged in modern Go to useos.ReadFileandos.WriteFiledirectly for new code. This helps ensure consistency and avoids reliance on soon-to-be fully deprecated APIs.Here’s a sample diff showing how you might replace
ioutilcalls:-import ( - "io/ioutil" - ... -) +import ( + "os" + ... +) -err := ioutil.WriteFile(sourceLockFilePath, []byte("lock file contents"), 0644) +err := os.WriteFile(sourceLockFilePath, []byte("lock file contents"), 0644) -contents, err := ioutil.ReadFile(destinationLockFilePath) +contents, err := os.ReadFile(destinationLockFilePath)util/file.go (1)
588-629: Consider concurrency safety if multiple processes run at the same time.
If there’s a chance of parallel execution, we could add locking or concurrency checks. That might prevent partial overwrites or race conditions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
util/file.go(2 hunks)util/file_test.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (3)
util/file_test.go (1)
344-454: Great coverage of lock file copying scenarios.
All your sub-tests nicely verify key conditions like missing files, unchanged contents, etc. This thorough approach helps ensure reliability.util/file.go (2)
4-4: Thanks for switching tobytes.Equal.
Comparing byte slices directly is more idiomatic and efficient than converting to strings.
588-629: Solid implementation aligned with the PR objective.
TheCopyLockFilefunction checks path equality, file existence, and content differences before copying. This avoids unnecessary writes, which matches Terraform’s approach and resolves read-only filesystem concerns.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for submitting this pull request. |
|
@denis256 we've been running these changes in production since this PR opened. Is there any chance these changes will get merged? |
ben-z
force-pushed
the
support-readonly-fs
branch
from
889aca5 to
6bcec9b
Compare
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@yhakbar updated the branch with the latest main. Thanks! |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/util.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
**/*.go: Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
config/util.go
🧬 Code Graph Analysis (1)
config/util.go (1)
util/file.go (1)
CopyLockFile(754-798)
🔇 Additional comments (1)
config/util.go (1)
14-14: LGTM - Clean delegation to utility function.The refactoring successfully delegates the lock file copying logic to the utility function, which implements the enhanced behavior described in the PR objectives (avoiding unnecessary copying when file contents are identical).
| func CopyLockFile(l log.Logger, opts *options.TerragruntOptions, sourceFolder, destinationFolder string) error { | ||
| sourceLockFilePath := util.JoinPath(sourceFolder, tf.TerraformLockFile) | ||
| destinationLockFilePath := util.JoinPath(destinationFolder, tf.TerraformLockFile) | ||
|
|
||
| if util.FileExists(sourceLockFilePath) { | ||
| l.Debugf("Copying lock file from %s to %s", sourceLockFilePath, destinationFolder) | ||
| return util.CopyFile(sourceLockFilePath, destinationLockFilePath) | ||
| } | ||
|
|
||
| return nil | ||
| return util.CopyLockFile(sourceFolder, destinationFolder, l) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider removing or utilizing the unused opts parameter.
The opts *options.TerragruntOptions parameter is not being passed to the utility function. This could indicate either:
- The parameter is no longer needed and should be removed from the function signature
- Some functionality from the options should still be utilized
Please verify that all necessary functionality is preserved and consider cleaning up the function signature if the parameter is truly unused.
If the opts parameter is no longer needed, apply this diff to clean up the function signature:
-func CopyLockFile(l log.Logger, opts *options.TerragruntOptions, sourceFolder, destinationFolder string) error {
+func CopyLockFile(l log.Logger, sourceFolder, destinationFolder string) error {
return util.CopyLockFile(sourceFolder, destinationFolder, l)
}Alternatively, if the options are still needed for some functionality, please ensure they are properly utilized in the delegation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func CopyLockFile(l log.Logger, opts *options.TerragruntOptions, sourceFolder, destinationFolder string) error { | |
| sourceLockFilePath := util.JoinPath(sourceFolder, tf.TerraformLockFile) | |
| destinationLockFilePath := util.JoinPath(destinationFolder, tf.TerraformLockFile) | |
| if util.FileExists(sourceLockFilePath) { | |
| l.Debugf("Copying lock file from %s to %s", sourceLockFilePath, destinationFolder) | |
| return util.CopyFile(sourceLockFilePath, destinationLockFilePath) | |
| } | |
| return nil | |
| return util.CopyLockFile(sourceFolder, destinationFolder, l) | |
| } | |
| func CopyLockFile(l log.Logger, sourceFolder, destinationFolder string) error { | |
| return util.CopyLockFile(sourceFolder, destinationFolder, l) | |
| } |
🤖 Prompt for AI Agents
In config/util.go around lines 13 to 15, the function CopyLockFile has an unused
parameter opts of type *options.TerragruntOptions. Review whether this parameter
is necessary for the function's operation; if it is not needed, remove opts from
the function signature and all calls to this function. If opts is required for
some functionality, update the call to util.CopyLockFile to include or utilize
opts appropriately to preserve intended behavior.
Description
Terragrunt copies lock files into the working directory every time Terraform is initialized. This is inconsistent with the Terraform behaviour where the lock file is only written to when changed.
This is important when the working directory is on a readonly filesystem. I work with an organization that does this to prevent unintentional code changes during provisioning.
This PR makes it so that copying the lock file is avoided when the file has no changes.
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Updated the lock file copy mechanism to avoid copying when the lock file is unchanged. Useful for using a readonly working directory.
Summary by CodeRabbit
Refactor
Tests