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

feat: support common modules #202

Merged
merged 5 commits into from
Mar 24, 2022

Conversation

henworth
Copy link
Contributor

Pull Request

Related Github Issues

Description

This adds support for the "common module" pattern that was recently added to the Terragrunt example repo. In addition it also automatically adds the included path as a dependency without requiring any extra_atlantis_dependencies blocks.

There is an important caveat to this approach however, every include block is added as an implicit dependency, including any parent module. This means that irrespective of the state of --ignore-parent-terragrunt the parent terragrunt.hcl will be included as there is no easy way differentiate between includes to find the "real" parent. For that reason I'm marking this as a draft as we will need something akin to the idea discussed in #155 in order to properly support that functionality.

I played around with the idea of adding a is_parent_terragrunt_file flag, but couldn't figure out the logic for it. That's the reason the --include-parent-changes cli argument still exists below, looks like I missed it in the change set.

This also adds a new functional cli argument --create-parent-project as I see a potential to want to include the parent as a dependency but not create a project for it. So these two arguments are the two components of those changes, with --ignore-parent-terragrunt disabling both. These should provide enough flexibility to cover most uses of parent modules.

Lastly, there are two minor changes to how the error handling works. First, the printing of the usage/help is now suppressed when exiting with an error. This was something that kept coming up during debugging and it made things a bit easier to view. Also, this removes the second printing of any error, again because this was cleaner during debugging.

Security Implications

  • [none]

System Availability

  • [none]

@henworth henworth changed the title feat: support common modules [WIP] feat: support common modules Feb 18, 2022
@dmattia
Copy link
Member

dmattia commented Feb 25, 2022

This looks really interesting @henworth, thank you so much for this. Are you able to go through and update the existing tests by chance to include the new outputs? It would be useful for me to review to go through example by example and see what all the net effects are on the e2e tests

@henworth
Copy link
Contributor Author

Absolutely I can do that. I just wanted to point it out before doing so.

cmd/generate.go Outdated Show resolved Hide resolved
cmd/generate.go Show resolved Hide resolved
@dmattia
Copy link
Member

dmattia commented Feb 25, 2022

@henworth I just went through and reviewed the code, and it's wonderful, thank you so much for this contribution.

every include block is added as an implicit dependency

I think this is sensible. It's definitely a change from how things have worked up until now with this tool, but I think with the evolution of terragrunt having multiple includes, this tradeoff is definitely necessary.

Once the tests pass here I will gladly merge + release 👍

@henworth
Copy link
Contributor Author

@dmattia I've updated all the test golden outputs, and they all pass on my local dev. I'll take a quick pass at addressing the other comments you have. Thanks!

@henworth
Copy link
Contributor Author

I'm not sure why the tests are failing. In my forked repo the task was successful: https://github.com/henworth/terragrunt-atlantis-config/actions/runs/1901169810

Looks like it is failing on the TestNotIgnoringParentTerragrunt test: https://github.com/transcend-io/terragrunt-atlantis-config/runs/5340383108?check_suite_focus=true#step:6:31, but locally I can't reproduce it.

@henworth henworth marked this pull request as ready for review February 28, 2022 15:19
@henworth henworth changed the title [WIP] feat: support common modules feat: support common modules Mar 1, 2022
@smitthakkar96
Copy link

@henworth did you get chance to come back to this? I badly need this feature :P

Hoping to see this releasing soon :)

@smitthakkar96
Copy link

I tried on my machine to reproduce however I am unable to as well, @dmattia can we please rerun the tests. Maybe a glitch or race condition that is not reproducible (if it doesn't work, retry :P), however it is better to find out

@henworth
Copy link
Contributor Author

henworth commented Mar 17, 2022

I've suspected something similar, and have been trying to debug it locally to find some strange edge case, but I can't find anything. Also considered the possibility of the version of Go itself causing the issue, but the test matrix is not consistent on which version will fail.

Yesterday I ran the test on a loop for about 2 hours and there were only two failures in about 14,000 iterations. I'm honestly stumped right now.

commit 9e0f741
Author: Mike Hennessy <henworth@henabytes.com>
Date:   Tue Mar 15 09:44:40 2022 -0400

    fix: remove createParentProject option

commit 6e8d245
Author: Mike Hennessy <henworth@henabytes.com>
Date:   Mon Mar 14 18:42:50 2022 -0400

    feat: bump base version to v1.16

commit 9118883
Author: Mike Hennessy <henworth@henabytes.com>
Date:   Mon Mar 14 17:33:39 2022 -0400

    chore: include go v1.17 in tests
@smitthakkar96
Copy link

smitthakkar96 commented Mar 17, 2022

Thanks for the quick response :)

Yesterday I ran the test on a loop for about 2 hours and there were only two failures in about 14,000 iterations. I'm honestly stumped right now.

Do you think we can do same for master. Maybe there is a hard to reproduce issue which is unrelated to your PR.

On a side note I wonder why we need to support multiple golang versions, it's not a library but more of a cli. People will simply download binary and run.

@henworth
Copy link
Contributor Author

And just like that... all the tests pass this time. ha

In regards to versions, I think it would be a good idea to drop tests for older non-supported versions of Go (1.13 and 1.14) and bump the "main" version to 1.16 to keep in step with Terragrunt. Atlantis is at 1.17 which I've added as a test version. Version 1.18 is also on the horizon which will cause 1.15 to drop out of support, so I think that could possibly be dropped as well.

@dmattia dmattia mentioned this pull request Mar 24, 2022
@dmattia dmattia self-requested a review March 24, 2022 18:07
Copy link
Member

@dmattia dmattia left a comment

Choose a reason for hiding this comment

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

Looks great, apologies for the test flakes, this is the first I've seen them 🤔

@dmattia dmattia merged commit c3df0de into transcend-io:master Mar 24, 2022
@smitthakkar96
Copy link

@dmattia did you create a release for this?

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.

None yet

3 participants