feat: Deprecating `terragrunt.hcl` as root #3588

yhakbar · 2024-11-21T23:16:46Z

Description

Making it so that usage of a root terragrunt.hcl throws a warning for users, and gives them guidance to move to a differently named root configuration file.

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)

Updated strict package.
Updated config package.
Updated scaffold command.
Updated catalog command.

Migration Guide

Current idiomatic usage of Terragrunt involves using a root configuration file named terragrunt.hcl.

This causes a lot of confusion for users, and potential errors. With this change, the new recommendation of naming it anything else will be enforced via the strict package.

It will likely take a very long time before this functionality can be removed, so we should start updating our documentation to reflect this change and throw warnings that users are using less reliable configurations.

…_parent_config("root")`

See this: https://circleci.com/docs/configuration-reference/#using-when-in-workflows

… `find_in_parent_folders("root.hcl")`

This reverts commit c0ecfb9.

test/integration_test.go

denis256 · 2024-12-06T16:56:31Z

test/integration_gcp_test.go

 	project := os.Getenv("GOOGLE_CLOUD_PROJECT")
 	gcsBucketName := "terragrunt-test-bucket-" + strings.ToLower(helpers.UniqueID())
-	tmpTerragruntGCSConfigPath := createTmpTerragruntGCSConfig(t, testFixtureGcsParallelStateInit, project, terraformRemoteStateGcpRegion, gcsBucketName, config.DefaultTerragruntConfigPath)
+	tmpTerragruntGCSConfigPath := createTmpTerragruntGCSConfig(t, testFixtureGcsParallelStateInit, project, terraformRemoteStateGcpRegion, gcsBucketName, "root.hcl")


Can config.RecommendedParentConfigName be used?

Ya, but it's a pain to update them all, and I don't think it's that important. I'll look to use that constant more consistently in the future.

levkohimins

You don't really need the Name field in strict.Control.

--- a/internal/strict/strict.go
+++ b/internal/strict/strict.go
@@ -21,8 +21,6 @@ import (
 // Control represents a control that can be enabled or disabled in strict mode.
 // When the control is enabled, Terragrunt will behave in a way that is not backwards compatible.
 type Control struct {
-	// Name is the name of the strict control.
-	Name string
 	// Error is the error that will be returned when the control is enabled.
 	Error error
 	// Warning is a warning that will be logged when the control is not enabled.
@@ -57,7 +55,7 @@ const (
 )
 
 // GetStrictControl returns the strict control with the given name.
-func GetStrictControl(name string) (Control, bool) {
+func GetStrictControl(name string) (*Control, bool) {
 	control, ok := StrictControls[name]
 
 	return control, ok
@@ -65,8 +63,8 @@ func GetStrictControl(name string) (Control, bool) {
 
 // Evaluate returns a warning if the control is not enabled, an indication of whether the control has already been triggered,
 // and an error if the control is enabled.
-func (control Control) Evaluate(opts *options.TerragruntOptions) (string, bool, error) {
-	_, triggered := TriggeredControls.LoadAndStore(control.Name, true)
+func (control *Control) Evaluate(opts *options.TerragruntOptions) (string, bool, error) {
+	_, triggered := TriggeredControls.LoadAndStore(control, true)
 
 	if opts.StrictMode {
 		return "", triggered, control.Error
@@ -88,73 +86,61 @@ func (control Control) Evaluate(opts *options.TerragruntOptions) (string, bool,
 	return control.Warning, triggered, nil
 }
 
-type Controls map[string]Control
+type Controls map[string]*Control
 
 //nolint:lll,gochecknoglobals,stylecheck,revive
 var StrictControls = Controls{
 	SpinUp: {
-		Name:    SpinUp,
 		Error:   errors.Errorf("The `%s` command is no longer supported. Use `terragrunt run-all apply` instead.", SpinUp),
 		Warning: fmt.Sprintf("The `%s` command is deprecated and will be removed in a future version. Use `terragrunt run-all apply` instead.", SpinUp),
 	},
 	TearDown: {
-		Name:    TearDown,
 		Error:   errors.Errorf("The `%s` command is no longer supported. Use `terragrunt run-all destroy` instead.", TearDown),
 		Warning: fmt.Sprintf("The `%s` command is deprecated and will be removed in a future version. Use `terragrunt run-all destroy` instead.", TearDown),
 	},
 	PlanAll: {
-		Name:    PlanAll,
 		Error:   errors.Errorf("The `%s` command is no longer supported. Use `terragrunt run-all plan` instead.", PlanAll),
 		Warning: fmt.Sprintf("The `%s` command is deprecated and will be removed in a future version. Use `terragrunt run-all plan` instead.", PlanAll),
 	},
 	ApplyAll: {
-		Name:    ApplyAll,
 		Error:   errors.Errorf("The `%s` command is no longer supported. Use `terragrunt run-all apply` instead.", ApplyAll),
 		Warning: fmt.Sprintf("The `%s` command is deprecated and will be removed in a future version. Use `terragrunt run-all apply` instead.", ApplyAll),
 	},
 	DestroyAll: {
-		Name:    DestroyAll,
 		Error:   errors.Errorf("The `%s` command is no longer supported. Use `terragrunt run-all destroy` instead.", DestroyAll),
 		Warning: fmt.Sprintf("The `%s` command is deprecated and will be removed in a future version. Use `terragrunt run-all destroy` instead.", DestroyAll),
 	},
 	OutputAll: {
-		Name:    OutputAll,
 		Error:   errors.Errorf("The `%s` command is no longer supported. Use `terragrunt run-all output` instead.", OutputAll),
 		Warning: fmt.Sprintf("The `%s` command is deprecated and will be removed in a future version. Use `terragrunt run-all output` instead.", OutputAll),
 	},
 	ValidateAll: {
-		Name:    ValidateAll,
 		Error:   errors.Errorf("The `%s` command is no longer supported. Use `terragrunt run-all validate` instead.", ValidateAll),
 		Warning: fmt.Sprintf("The `%s` command is deprecated and will be removed in a future version. Use `terragrunt run-all validate` instead.", ValidateAll),
 	},
 	SkipDependenciesInputs: {
-		Name:    SkipDependenciesInputs,
 		Error:   errors.Errorf("The `%s` option is deprecated. Reading inputs from dependencies has been deprecated and will be removed in a future version of Terragrunt. To continue using inputs from dependencies, forward them as outputs.", SkipDependenciesInputs),
 		Warning: fmt.Sprintf("The `%s` option is deprecated and will be removed in a future version of Terragrunt. Reading inputs from dependencies has been deprecated. To continue using inputs from dependencies, forward them as outputs.", SkipDependenciesInputs),
 	},
 	DisableLogFormatting: {
-		Name:    DisableLogFormatting,
 		Error:   errors.Errorf("The `--%s` flag is no longer supported. Use `--terragrunt-log-format=key-value` instead.", DisableLogFormatting),
 		Warning: fmt.Sprintf("The `--%s` flag is deprecated and will be removed in a future version. Use `--terragrunt-log-format=key-value` instead.", DisableLogFormatting),
 	},
 	JSONLog: {
-		Name:    JSONLog,
 		Error:   errors.Errorf("The `--%s` flag is no longer supported. Use `--terragrunt-log-format=json` instead.", JSONLog),
 		Warning: fmt.Sprintf("The `--%s` flag is deprecated and will be removed in a future version. Use `--terragrunt-log-format=json` instead.", JSONLog),
 	},
 	TfLogJSON: {
-		Name:    TfLogJSON,
 		Error:   errors.Errorf("The `--%s` flag is no longer supported. Use `--terragrunt-log-format=json` instead.", TfLogJSON),
 		Warning: fmt.Sprintf("The `--%s` flag is deprecated and will be removed in a future version. Use `--terragrunt-log-format=json` instead.", TfLogJSON),
 	},
 	RootTerragruntHCL: {
-		Name:    RootTerragruntHCL,
 		Error:   errors.Errorf("Using `terragrunt.hcl` as the root of Terragrunt configurations is an anti-pattern, and no longer supported. Use a differently named file like `root.hcl` instead. For more information, see https://terragrunt.gruntwork.io/docs/migrate/migrating-from-root-terragrunt-hcl"),
 		Warning: "Using `terragrunt.hcl` as the root of Terragrunt configurations is an anti-pattern, and no longer recommended. In a future version of Terragrunt, this will result in an error. You are advised to use a differently named file like `root.hcl` instead. For more information, see https://terragrunt.gruntwork.io/docs/migrate/migrating-from-root-terragrunt-hcl",
 	},
 }
 
-var TriggeredControls = xsync.NewMapOf[string, bool]()
+var TriggeredControls = xsync.NewMapOf[*Control, bool]()
 
 // Names returns the names of all strict controls.
 func (controls Controls) Names() []string {

levkohimins · 2024-12-08T14:37:54Z

internal/strict/strict.go

 }

+var TriggeredControls = xsync.NewMapOf[string, bool]()


Global variable can cause problems in tests, especially with -race.

Ya, but does it really matter?

As long as we test the stderr check integration test out of parallel, we should be fine, right?
https://github.com/gruntwork-io/terragrunt/pull/3588/files#diff-4e1bb7ad3f548fa9a54e7e0f4b57c046d000c335ba4802df083a8e93416d5abfR85

In unit tests, I just ignore the triggered status:
https://github.com/gruntwork-io/terragrunt/pull/3588/files#diff-5e4aa2fd41e755849a1962333f14613e6189d0aaf00d10eac0a756cebe1692ccR68

Until we create another test to check the strict-mode (in config or CLI), this should work stably. It's up to you, we just need remember this.

yhakbar · 2024-12-09T21:11:16Z

I'm moving this into draft status, as we are discussing internally on the best way to roll this out and communicate what's going to happen to Terragrunt as a consequence of this.

yhakbar · 2024-12-09T21:15:57Z

You don't really need the Name field in strict.Control.

I considered this, but I wonder if it's actually better. I don't like Golang pointers as it's easy to create nil pointer exceptions, and I wouldn't want to put the whole struct in the triggered map.

Is it that bad to duplicate the name of the control in the value found by the key?

levkohimins · 2024-12-10T00:04:28Z

You don't really need the Name field in strict.Control.

I considered this, but I wonder if it's actually better. I don't like Golang pointers as it's easy to create nil pointer exceptions, and I wouldn't want to put the whole struct in the triggered map.

Is it that bad to duplicate the name of the control in the value found by the key?

Not sure if it's a good idea to duplicate the same string data in the map key and field structure, I see two options:

What I already suggested (I don't see anything wrong with using a pointer).
Remake StrictControls from map to slice.

yhakbar · 2024-12-13T21:12:33Z

docs/_docs/04_reference/built-in-functions.md

@@ -91,21 +91,15 @@ If there is a specific function you would like to see supported directly in Terr

 ## find_in_parent_folders


Add in a note on the historical legacy behavior of no params.

yhakbar force-pushed the feat/deprecate-terragrunt-hcl-as-root branch 2 times, most recently from b25c288 to 4d12fbe Compare December 2, 2024 23:16

yhakbar added 23 commits December 3, 2024 17:17

feat: Deprecating terragrunt.hcl as root

49b3a89

fix: Passing in Logger to catalog

3f86768

fix: Updating all references to find_in_parent_config() to `find_in…

4ab7b1b

…_parent_config("root")`

fix: Addressing markdownlint errors

9d48306

fix: Addressing deprecated CircleCI configs.

e75210b

See this: https://circleci.com/docs/configuration-reference/#using-when-in-workflows

fix: Adjusting usage of find_in_parent_folders("terragrunt.hcl") to…

2cc070f

… `find_in_parent_folders("root.hcl")`

Revert "fix: Addressing deprecated CircleCI configs."

a4040b6

This reverts commit c0ecfb9.

fix: Renaming more root terragrunt.hcl files

7722fe6

fix: Test fixes

5e94730

fix: Addressing some hidden dependencies on root terragrunt.hcl

948af67

fix: Addressing more hidden root terragrunt.hcl usage

3264a04

fix: Using non-deprecated resource instead of data

9408b4a

fix: Using non-deprecated run-all instead of apply-all

01711b8

fix: Fixing more tests

4c9ddc2

fix: Setting root as root.hcl in AWS and GCP tests

e3e2112

fix: Removing unneeded config

d57c894

fix: Adjusting which tests use root.hcl and which use terragrunt.hcl

7aaaf21

fix: Hopefully last config adjustment

a7e8d7f

fix: Another unit test fix

532c309

fix: Adding debounce to strict controls

53cb839

wip: Working on getting scaffold to allow for non-terragrunt.hcl roots

e478cb9

fix: Explicitly ignoring triggered status during tests

4191c33

fix: Fixing more tests

4ab39f9

yhakbar force-pushed the feat/deprecate-terragrunt-hcl-as-root branch from e003d54 to 4ab39f9 Compare December 3, 2024 22:19

fix: Fixing TestDefaultTemplateVariables

fd3c255

yhakbar commented Dec 4, 2024

View reviewed changes

test/integration_test.go Show resolved Hide resolved

yhakbar added 2 commits December 4, 2024 11:49

fix: Fixing more tests

1520a06

fix: Remove strict mode integration tests from parallel

09cc43f

yhakbar added 3 commits December 4, 2024 14:27

fix: Fixing manually setting the variable for scaffold

936dbcf

feat: More documentation and testing

9fe597d

fix: Made an adjustment I'm not sure about

3bba7d0

yhakbar marked this pull request as ready for review December 5, 2024 15:10

yhakbar requested review from denis256 and levkohimins as code owners December 5, 2024 15:10

denis256 approved these changes Dec 6, 2024

View reviewed changes

levkohimins requested changes Dec 8, 2024

View reviewed changes

levkohimins reviewed Dec 8, 2024

View reviewed changes

yhakbar marked this pull request as draft December 9, 2024 21:10

yhakbar commented Dec 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Deprecating `terragrunt.hcl` as root #3588

feat: Deprecating `terragrunt.hcl` as root #3588

yhakbar commented Nov 21, 2024 •

edited

Loading

denis256 Dec 6, 2024

yhakbar Dec 6, 2024

denis256 Dec 6, 2024

levkohimins left a comment •

edited

Loading

levkohimins Dec 8, 2024

yhakbar Dec 9, 2024

levkohimins Dec 10, 2024

yhakbar commented Dec 9, 2024

yhakbar commented Dec 9, 2024

levkohimins commented Dec 10, 2024

yhakbar Dec 13, 2024

		@@ -91,21 +91,15 @@ If there is a specific function you would like to see supported directly in Terr

		## find_in_parent_folders

feat: Deprecating terragrunt.hcl as root #3588

Are you sure you want to change the base?

feat: Deprecating terragrunt.hcl as root #3588

Conversation

yhakbar commented Nov 21, 2024 • edited Loading

Description

TODOs

Release Notes (draft)

Migration Guide

denis256 Dec 6, 2024

Choose a reason for hiding this comment

yhakbar Dec 6, 2024

Choose a reason for hiding this comment

denis256 Dec 6, 2024

Choose a reason for hiding this comment

levkohimins left a comment • edited Loading

Choose a reason for hiding this comment

levkohimins Dec 8, 2024

Choose a reason for hiding this comment

yhakbar Dec 9, 2024

Choose a reason for hiding this comment

levkohimins Dec 10, 2024

Choose a reason for hiding this comment

yhakbar commented Dec 9, 2024

yhakbar commented Dec 9, 2024

levkohimins commented Dec 10, 2024

yhakbar Dec 13, 2024

Choose a reason for hiding this comment

feat: Deprecating `terragrunt.hcl` as root #3588

feat: Deprecating `terragrunt.hcl` as root #3588

yhakbar commented Nov 21, 2024 •

edited

Loading

levkohimins left a comment •

edited

Loading