Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP - feat: introducing "copy_terraform_lock_file" to fine tune Lock File Handling #2889

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

rodrigorfk
Copy link
Contributor

@rodrigorfk rodrigorfk commented Jan 15, 2024

Description

Fixes #1653.

This PR introduces a new attribute called copy_terraform_lock_file under the terraform block as following:

terraform {
  ...
  copy_terraform_lock_file = false
}

The default value is true and does not change any existing Terragrunt behaviour, once it is defined as false, Terragrunt will stop copying the .terraform.lock.hcl file from the temporary terragrunt directory to the working directory.

TODOs

Read the Gruntwork contribution guidelines.

  • Update the docs.
  • Run the relevant tests successfully, including pre-commit checks.
  • Ensure any 3rd party code adheres with our license policy or delete this line if its not applicable.
  • Include release notes. If this PR is backward incompatible, include a migration guide.

Release Notes (draft)

Added / Removed / Updated [X].

Added "copy_terraform_lock_file" attribute to fine tune Lock File Handling

Migration Guide

@rodrigorfk
Copy link
Contributor Author

Hi @denis256, would you be so kind and provide some feedback about this PR? Thanks in advance.

@denis256
Copy link
Member

Hi, yes, it is in my queue to check... ⌛

@rodrigorfk
Copy link
Contributor Author

Hi @denis256 , sorry to bother you once again, but is there anything I could help you to expedite the review of this PR? I would love to hear from you if this change is something the project is willing to accept. Thanks once again.

@Josephuss
Copy link

hello @rodrigorfk @denis256 , hope you're doing well. Can I ask if this merge can be completed sometime soon?

my team found this merge request and we could all use this feature since we're trying to create a remote and client setup for terragrunt and versioning could become an issue if there are locks on both remote and client side.

# Conflicts:
#	cli/commands/terraform/action.go
@rodrigorfk
Copy link
Contributor Author

Hi @Josephuss , thanks for your message, I'm happy to see that your team also thought this change would be useful, I have updated the branch and fixed the outstanding conflicts.

@james-masson
Copy link

We've also come across this PR, it's exactly the functionality we need, for the same reasons as the others mentioned.

@IvanKuzyshyn
Copy link

Thanks @rodrigorfk for bringing this update! This change unlocks the ability for the lock file to be managed and versioned by the remove module itself. It would be great to have it merged.

# Conflicts:
#	config/include_test.go
#	docs/_docs/02_features/lock-file-handling.md
Signed-off-by: Rodrigo Fior Kuntzer <[email protected]>
@rodrigorfk
Copy link
Contributor Author

Thanks for the interest @IvanKuzyshyn , I have updated the branch and resolved the conflicts once again. Hopefully @denis256 will have some time to provide a feedback about this change soon. 🙏

Signed-off-by: Rodrigo Fior Kuntzer <[email protected]>
Copy link

sonarcloud bot commented Jun 16, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
1.8% Duplication on New Code

See analysis details on SonarCloud

@gmauleon
Copy link

gmauleon commented Jul 9, 2024

Just passing by to nudge this as I'm also interested, thanks in advance 😀

Copy link
Collaborator

@yhakbar yhakbar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! I left some small details that can be addressed before we get this merged in.

Sorry we've let this PR get stale. I appreciate that the tests were updated, and docs were improved with the feature update.

func shouldCopyLockFile(args []string) bool {
func shouldCopyLockFile(args []string, terraformConfig *config.TerraformConfig) bool {
if terraformConfig != nil && terraformConfig.CopyTerraformLockFile != nil && !*terraformConfig.CopyTerraformLockFile {
return false
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be simplified as return *terraformConfig.CopyTerraformLockFile, with the last part of the if statement removed above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the suggestion @yhakbar, but I am not sure if we should perform an early return like that, if we do that and the user explicitly defines copyTerraformLockFile = true on their terragrunt.hcl, terragrunt will copy the file on all commands, which will change the existing behaviour about copying it only for the init and providers lock command. Thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good point. It might help to leave a comment above the check to make sure that someone doesn't make the same logical mistake I'm making.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done @yhakbar, added the comment, do you mind having another look? thanks in advance

docs/_docs/02_features/lock-file-handling.md Outdated Show resolved Hide resolved
docs/_docs/02_features/lock-file-handling.md Outdated Show resolved Hide resolved
docs/_docs/02_features/lock-file-handling.md Outdated Show resolved Hide resolved
docs/_docs/04_reference/config-blocks-and-attributes.md Outdated Show resolved Hide resolved
@rodrigorfk
Copy link
Contributor Author

Hi @yhakbar, thanks for the review and suggestions, I will be away for a week and will make sure I go over your comments once I am back.

@yhakbar yhakbar changed the title feat: introducing "copy_terraform_lock_file" to fine tune Lock File Handling WIP - feat: introducing "copy_terraform_lock_file" to fine tune Lock File Handling Aug 13, 2024
# Conflicts:
#	config/config_as_cty.go
#	docs/_docs/04_reference/config-blocks-and-attributes.md
Signed-off-by: Rodrigo Fior Kuntzer <[email protected]>
Signed-off-by: Rodrigo Fior Kuntzer <[email protected]>
Copy link

sonarcloud bot commented Aug 19, 2024

@gmauleon
Copy link

Hey there kind folks, just coming to bump that up the grave if anyone is available for review 🙏

@yhakbar
Copy link
Collaborator

yhakbar commented Sep 19, 2024

Hey @gmauleon , this PR currently has some conflicts, so it'll need those conflicts resolved, lints passing and tests run before it can be reviewed. When it is, the title should be updated, with reviews requested.

If you could help out @rodrigorfk by supplying a PR to his source branch, that might help things move along.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lock file handling strategy is not best practice?
7 participants