feat: support merging the "skip" attribute from included files. #3225

rodrigorfk · 2024-06-24T10:03:46Z

Description

This PR is introducing support for merging the content of the skip attribute from include blocks, in the same way of many other attributes and blocks are already being merged.

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].

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

denis256 · 2024-06-24T14:37:55Z

-- FAIL: TestResolveTerraformModulesReadConfigFromParentConfig (0.02s)

Expected :config.TerragruntConfig{Catalog:(*config.CatalogConfig)(nil), Terraform:(*config.TerraformConfig)(0xc0006abf10), TerraformBinary:"", TerraformVersionConstraint:"", TerragruntVersionConstraint:"", RemoteState:(*remote.RemoteState)(nil), Dependencies:( ...

Actual   :config.TerragruntConfig{Catalog:(*config.CatalogConfig)(nil), Terraform:(*config.TerraformConfig)(0xc000129260), TerraformBinary:"", TerraformVersionConstraint:"", TerragruntVersionConstraint:"", RemoteState:(*remote.RemoteState)(nil), Dependencies:( ...

denis256 · 2024-06-24T14:43:03Z

Integration test TestApplyAllSkipTrue also seems to fail

    integration_test.go:4314: Copying fixture-skip/ to /tmp/terragrunt-test2863889219
    integration_test.go:4268: [terragrunt apply-all --terragrunt-non-interactive --terragrunt-working-dir /tmp/terragrunt-test2863889219/fixture-skip/skip-true --terragrunt-log-level info]
    integration_test.go:2010: [TestApplyAllSkipTrue] Full contents of show stdout:
    integration_test.go:2010: [TestApplyAllSkipTrue] 
    integration_test.go:2011: [TestApplyAllSkipTrue] Full contents of show stderr:
    integration_test.go:2011: [TestApplyAllSkipTrue] time=2024-06-24T15:42:21+01:00 level=warning msg='apply-all' is deprecated. Running 'terragrunt run-all apply' instead. Please update your workflows to use 'terragrunt run-all apply', as 'apply-all' may be removed in the future!
    integration_test.go:2011: [TestApplyAllSkipTrue] 
    integration_test.go:2011: [TestApplyAllSkipTrue] time=2024-06-24T15:42:21+01:00 level=info msg=The stack at /tmp/terragrunt-test2863889219/fixture-skip/skip-true will be processed in the following order for command apply:
    integration_test.go:2011: [TestApplyAllSkipTrue] Group 1
    integration_test.go:2011: [TestApplyAllSkipTrue] - Module /tmp/terragrunt-test2863889219/fixture-skip/skip-true
    integration_test.go:2011: [TestApplyAllSkipTrue] - Module /tmp/terragrunt-test2863889219/fixture-skip/skip-true/resource1
    integration_test.go:2011: [TestApplyAllSkipTrue] - Module /tmp/terragrunt-test2863889219/fixture-skip/skip-true/resource2
    integration_test.go:2011: [TestApplyAllSkipTrue] 
    integration_test.go:2011: [TestApplyAllSkipTrue] 
    integration_test.go:2011: [TestApplyAllSkipTrue] time=2024-06-24T15:42:21+01:00 level=info msg=Skipping terragrunt module /tmp/terragrunt-test2863889219/fixture-skip/skip-true/terragrunt.hcl due to skip = true. prefix=[/tmp/terragrunt-test2863889219/fixture-skip/skip-true] 
    integration_test.go:2011: [TestApplyAllSkipTrue] time=2024-06-24T15:42:21+01:00 level=info msg=Skipping terragrunt module /tmp/terragrunt-test2863889219/fixture-skip/skip-true/resource2/terragrunt.hcl due to skip = true. prefix=[/tmp/terragrunt-test2863889219/fixture-skip/skip-true/resource2] 
    integration_test.go:2011: [TestApplyAllSkipTrue] time=2024-06-24T15:42:21+01:00 level=info msg=Skipping terragrunt module /tmp/terragrunt-test2863889219/fixture-skip/skip-true/resource1/terragrunt.hcl due to skip = true. prefix=[/tmp/terragrunt-test2863889219/fixture-skip/skip-true/resource1] 
    integration_test.go:2011: [TestApplyAllSkipTrue] 
    integration_test.go:2018: 
        	Error Trace:	/home/denis256/projects/gruntwork/terragrunt/test/integration_test.go:2018
        	Error:      	"" does not contain "hello, Ernie"
        	Test:       	TestApplyAllSkipTrue
    integration_test.go:2019: 
        	Error Trace:	/home/denis256/projects/gruntwork/terragrunt/test/integration_test.go:2019
        	Error:      	"" does not contain "hello, Bert"
        	Test:       	TestApplyAllSkipTrue

denis256 · 2024-06-24T14:53:24Z

docs/_docs/04_reference/config-blocks-and-attributes.md

-The `skip` flag must be set explicitly in terragrunt modules that should be skipped. If you set `skip = true` in a
-`terragrunt.hcl` file that is included by another `terragrunt.hcl` file, only the `terragrunt.hcl` file that explicitly
-set `skip = true` will be skipped.
+The `skip` flag can be inherited from an included `terragrunt.hcl` file if `skip` is defined there, unless it is


I think may be helpful to add an integration test for this case, when the skip flag is inherited from include

Thanks for your suggestion @denis256 , I've updated the TestApplyAllSkipTrue integration test to cover this scenario as well, it was already using an partial terragrunt file there with a skip = true defined, so now one of the child modules is inheriting the skip and the other is overwriting it.

…handle the case where skip is inherited from the included terragrunt file Signed-off-by: Rodrigo Fior Kuntzer <[email protected]>

rodrigorfk · 2024-06-24T15:16:22Z

Thanks for publishing the failed tests here @denis256 and sorry for not catching them initially, I've fixed them.

rodrigorfk · 2024-06-26T08:49:00Z

Hi @denis256 , could you please share the reason why the test_signing job failed? looking into it's definition it seems to only be related to binary signing and not really related to the specific change I am doing in this PR, right? thanks in advance.

denis256 · 2024-06-26T09:19:02Z

Hi,
it is an internal issue with Apple... I hope will be fixed soon

🍎 🪱

yhakbar · 2024-08-09T13:05:05Z

Hey @rodrigorfk ,

Do you mind rebasing this PR? I'd like to avoid having it go stale.

# Conflicts: # config/config_test.go # config/dependency.go # config/include.go # config/include_test.go # configstack/module_test.go # test/integration_test.go

rodrigorfk · 2024-08-16T10:56:24Z

done @yhakbar, the latest changes from master were merged into this branch.

yhakbar · 2024-08-16T18:07:35Z

Hey @rodrigorfk ,

The PR is failing CI checks. Did you run checks locally?

You should be able to run make install-lint && make run-lint without needing access to AWS or anything.

We're working on making it easier to run tests locally. I know it's not as straightforward as it could be. If you could make sure to execute the tests that you're modifying though, that will speed up the process on this review.

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

sonarcloud · 2024-08-19T08:29:11Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

rodrigorfk · 2024-08-19T08:31:15Z

Hey @yhakbar,

Sorry for the issue, forgot to check it locally after the merge, mostly because as you said, it is pretty hard to run all the test suite locally, but anyway, it shouldn't be an excuse for not running focused ones, I've fixed it and I hope it works now.

denis256 · 2024-09-02T18:10:56Z

Looks like some tests are still failing:
TestResolveTerraformModulesReadConfigFromParentConfig
TestTableTagging

yhakbar · 2024-09-17T20:27:34Z

Sorry we haven't been able to get this PR merged, @rodrigorfk .

To avoid having it stay stale, would you mind resolving conflicts, then ensure that the tests @denis256 mentioned pass locally?

yhakbar · 2024-09-25T13:12:05Z

This pull request is still stale. I'm going to close it out.

If you would like to open it again, please make sure to rebase, lint and test locally, then request re-review.

# Conflicts: # config/include.go

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

rodrigorfk · 2024-09-26T07:15:15Z

Hi @yhakbar, sorry for missing your request for rebase and failed tests before, I have done it right now and the branch was updated, do you mind re-opening the PR? or shall I create a new one? Thanks in advance.

asvinours · 2024-10-10T15:05:07Z

this feature is introducing a major breaking change for all existing setup, if the parent file has a skip flag, every single module is now being skipped. This means terragrunt has become a no-op for a lot of our repositories over night

yhakbar · 2024-10-10T15:19:28Z

@asvinours ,

It is a breaking change, and was called out in the release notes.

Why does the included configuration define a skip? Can you give some context as to why you have it defined there, but don't want it included?

asvinours · 2024-10-10T15:32:25Z

We use a parent/child structure for our terragrunt config so we can centralize a lot of the shared logic in the parent file and then include it in all the child files. Each child file correspond to an actual infrastructure module, whereas the parent file is just there to be included and isn't tied to any infra code.

Once I saw terragrunt was skipping all the work I looked at the most recent releases and noticed the changelog but the issue here is terragrunt version doesn't use the major flag of semver, only minor and patch. So as soon as 0.68.0 was released our CICD pipelines pulled down the latest version and our pipelines broke :(

We're updating our version locks to < 0.68.0 for now until we have time to adapt all our code to this new behaviour

feat: support merging the "skip" attribute from included files.

82ab199

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

rodrigorfk requested a review from denis256 as a code owner June 24, 2024 10:03

denis256 reviewed Jun 24, 2024

View reviewed changes

test: fix failing tests and prepare the TestApplyAllSkipTrue test to …

13c9be2

…handle the case where skip is inherited from the included terragrunt file Signed-off-by: Rodrigo Fior Kuntzer <[email protected]>

Merge branch 'refs/heads/master' into feature/merge-skip

b8e4b52

# Conflicts: # config/config_test.go # config/dependency.go # config/include.go # config/include_test.go # configstack/module_test.go # test/integration_test.go

rodrigorfk requested review from levkohimins and yhakbar as code owners August 16, 2024 10:55

fix: post merge fixes

237c4b2

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

yhakbar closed this Sep 25, 2024

rodrigorfk added 2 commits September 26, 2024 09:02

Merge branch 'refs/heads/main' into feature/merge-skip

4fb71c6

# Conflicts: # config/include.go

test: fix failing tests

2ca2622

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

yhakbar reopened this Sep 26, 2024

yhakbar approved these changes Oct 7, 2024

View reviewed changes

yhakbar merged commit 38c8f29 into gruntwork-io:main Oct 7, 2024
5 checks passed

BrewTestBot mentioned this pull request Oct 9, 2024

terragrunt 0.68.0 Homebrew/homebrew-core#193411

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support merging the "skip" attribute from included files. #3225

feat: support merging the "skip" attribute from included files. #3225

rodrigorfk commented Jun 24, 2024

denis256 commented Jun 24, 2024

denis256 commented Jun 24, 2024

denis256 Jun 24, 2024

rodrigorfk Jun 24, 2024 •

edited

Loading

rodrigorfk commented Jun 24, 2024

rodrigorfk commented Jun 26, 2024

denis256 commented Jun 26, 2024

yhakbar commented Aug 9, 2024

rodrigorfk commented Aug 16, 2024

yhakbar commented Aug 16, 2024

sonarcloud bot commented Aug 19, 2024

rodrigorfk commented Aug 19, 2024

denis256 commented Sep 2, 2024

yhakbar commented Sep 17, 2024

yhakbar commented Sep 25, 2024

rodrigorfk commented Sep 26, 2024

asvinours commented Oct 10, 2024

yhakbar commented Oct 10, 2024

asvinours commented Oct 10, 2024

feat: support merging the "skip" attribute from included files. #3225

feat: support merging the "skip" attribute from included files. #3225

Conversation

rodrigorfk commented Jun 24, 2024

Description

TODOs

Release Notes (draft)

denis256 commented Jun 24, 2024

denis256 commented Jun 24, 2024

denis256 Jun 24, 2024

Choose a reason for hiding this comment

rodrigorfk Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

rodrigorfk commented Jun 24, 2024

rodrigorfk commented Jun 26, 2024

denis256 commented Jun 26, 2024

yhakbar commented Aug 9, 2024

rodrigorfk commented Aug 16, 2024

yhakbar commented Aug 16, 2024

sonarcloud bot commented Aug 19, 2024

Quality Gate passed

rodrigorfk commented Aug 19, 2024

denis256 commented Sep 2, 2024

yhakbar commented Sep 17, 2024

yhakbar commented Sep 25, 2024

rodrigorfk commented Sep 26, 2024

asvinours commented Oct 10, 2024

yhakbar commented Oct 10, 2024

asvinours commented Oct 10, 2024

rodrigorfk Jun 24, 2024 •

edited

Loading