- Notifications
You must be signed in to change notification settings - Fork913
feat: warn when .terraform.lock.hcl is modified during terraform init#18276
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This change addresses issue#18237 by adding checksum validation for.terraform.lock.hcl files before and after running terraform init.When the lock file is modified during init, it indicates that providerhashes may be missing for the target architecture, leading to unnecessaryprovider downloads and slower provisioning.Changes:- Add calculateFileChecksum() helper function using SHA256- Add getTerraformLockFilePath() helper function- Modify init() function to calculate checksums before/after terraform init- Log warning when lock file changes with actionable guidance- Add unit tests for new functionalityThe warning message guides users to regenerate the lock file on the sameOS/architecture as their Coder instance to improve performance.Co-authored-by: kylecarbs <7122116+kylecarbs@users.noreply.github.com>
Co-authored-by: kylecarbs <7122116+kylecarbs@users.noreply.github.com>
Co-authored-by: kylecarbs <7122116+kylecarbs@users.noreply.github.com>
Co-authored-by: kylecarbs <7122116+kylecarbs@users.noreply.github.com>
Fixes gosec G306 linting error that expects WriteFile permissionsto be 0600 or less for security reasons.Co-authored-by: kylecarbs <7122116+kylecarbs@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There's lots of other reasons for modifications to the lock file. The current wording we add could potentially send folks down a rabbit hole.
What if we instead:
Logged thediff of the lock file to provide additional troubleshooting information,
Linked to the official documentationhttps://developer.hashicorp.com/terraform/language/files/dependency-lock#understanding-lock-file-changes so that folks can figure out the potential reason on their own?
Great feedback@johnstcn! You're absolutely right that there are many reasons for lock file modifications and the current warning could be misleading. I'll implement your suggestions:
This approach will be more helpful and less presumptuous about the cause. Let me update the implementation accordingly. |
Based on review feedback from@johnstcn, this commit improves theterraform lock file change detection by:1. **Logging actual diff**: Instead of just checksums, now logs the actual differences in the lock file, providing actionable troubleshooting information.2. **Neutral messaging**: Changed from WARNING to INFO level with neutral language that doesn't assume the cause of changes.3. **Official documentation**: Links to HashiCorp's official documentation about lock file changes instead of making assumptions about the cause.4. **Better user experience**: Users get helpful diff output and can understand what changed and why, rather than being sent down a potentially incorrect troubleshooting path.The implementation now:- Saves the original lock file before terraform init- Generates a simple diff showing added/removed lines- Provides informative messaging with official docs link- Includes comprehensive test coverageCo-authored-by: kylecarbs <7122116+kylecarbs@users.noreply.github.com>
✅Implemented the suggested improvements! I've updated the implementation based on your excellent feedback: Changes Made1.Actual Diff Logging 📊
2.Neutral Messaging ℹ️
3.Official Documentation Link 📚
4.Better Implementation 🔧
Example OutputThis approach is much more helpful and doesn't make assumptions about why the lock file changed. Thanks for the great suggestions! 🙏 |
| // Copy the lock file if it exists | ||
| if lockFileData, err := os.ReadFile(lockFilePath); err == nil { | ||
| _ = os.WriteFile(preInitLockFile, lockFileData, 0644) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
- This needs to be formatted speficially as octal, e.g.:
-_ = os.WriteFile(preInitLockFile, lockFileData, 0644)+_ = os.WriteFile(preInitLockFile, lockFileData, 0o644)I don't think we should be writing to the work directory directly. If we need to write a file, we should write to a temporary directory instead. Do weactually need to persist the lock file content pre-init? Can't we just store in memory instead?
If we write a file to disk, we need to ensure we use minimal permissions (0o600) or the
goseclinter will complain.
There was a bug with this chat. Closing this PR in favor of the new. |
Fixes#18237
This PR adds checksum validation for
.terraform.lock.hclfiles before and after runningterraform initto detect when provider hashes are missing for the target architecture.Problem
When users run
terraform initlocally on a different OS/architecture than their Coder instance, the generated.terraform.lock.hclfile may be missing provider hashes for the target architecture. This causes Terraform to download providers unnecessarily during provisioning, slowing down the process.Solution
.terraform.lock.hclbefore runningterraform initterraform initcompletesChanges
calculateFileChecksum()helper function using SHA256getTerraformLockFilePath()helper functioninit()function to perform checksum validationTesting
The warning message provides clear guidance on how to resolve the issue by regenerating the lock file on the same OS/architecture as the Coder instance.