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

[RFC] Imports: soliciting feedback from community #1025

Merged
merged 11 commits into from Feb 27, 2020

Conversation

yorinasub17
Copy link
Contributor

@yorinasub17 yorinasub17 commented Jan 30, 2020

This is an RFC that introduces import blocks for addressing limitations of configuration reuse with include.

I would like feedback from the entire community on this, as this has the potential to address a lot of limitations that were proposed throughout the lifetime of terragrunt, but is fairly opinionated in the design principle.

NOTE: It may be easier to read in the rendered form. Here is the direct link.

yorinasub17 added 2 commits Jan 30, 2020
…mplement a few easy to understand functions that are in the context of the child?
- `path_relative_to_import` and `path_relative_from_import`: These are the equivalent functions of the ones named
for `include`.
- `find_in_parent_folders_from_importing_config`: This function is the version of `find_in_parent_folders` that
works in the context of the config that is importing the current config.
Copy link
Contributor Author

@yorinasub17 yorinasub17 Jan 30, 2020

Choose a reason for hiding this comment

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

Use case that would benefit heavily from this: #744 (comment)

@brikis98
Copy link
Member

brikis98 commented Jan 30, 2020

Thanks for the great write-up Yori. I love going through all the concrete use cases to make sure it's solving the right problems 👍

Of the "reuse" options on the table—that is, the existing include, globals, read_terragrunt_config, manually reading JSON/YAML files, and your new import idea—I think import is probably the best bet, with read_terragrunt_config not too far behind (though I'd want to go through all the explicit use cases with it to see what they look like). However, there is one more option to consider that isn't explicitly about "reuse", which is the proposal I made here: #759.

It seems like with that approach, most of the code reuse issues go away. Everything in one environment is in one file (or, more likely, one set of .hcl files in a single folder), you can use locals to share variables, you can use module.xxx expressions to express dependencies, you don't need too many special Terragrunt helpers, and so on. In short, it's almost vanilla Terraform, which I'd argue is a very good thing, but with some extra functionality for better permissions management and avoiding everything ending up in one giant state file. If we're going to make a large change, this approach still strikes me as worth considering. How does it handle the code reuse use cases we are looking at with merge?

@tomasbackman
Copy link

tomasbackman commented Feb 5, 2020

Yes, very good write up and summary. Im really looking forward to having any of these solutions come to life as the current state is a bit annoying with the limitations in reusability.
Personally I would be happy with any of the proposed solutions. Most important for me is that they do not take too long to get.. terragrunt is after all a great wrapper that makes terraform easier/better. So letting these limitations stay too long feels worse (and contradicting the goal more) for me than getting the slightly "wrong" implementation (that still solves the issue.. =)

@yorinasub17
Copy link
Contributor Author

yorinasub17 commented Feb 5, 2020

Thanks for the feedback so far!

However, there is one more option to consider that isn't explicitly about "reuse" ...

Ah I was thinking of that as orthogonal, but you bring up a good point that it is related. When I get back to more thinking on this, I'll update with a section discussing this alternative.

I think import is probably the best bet, with read_terragrunt_config not too far behind (though I'd want to go through all the explicit use cases with it to see what they look like)

Given that read_terragrunt_config will probably be the easiest to implement, and to @tomasbackman point about speed of delivery and not letting this problem linger, I think it is worth baking that a bit more.


Given the above two and the feedback I've received, next steps on this RFC are for me to:

@jfunnell
Copy link

jfunnell commented Feb 6, 2020

I agree with @brikis98 for what it's worth.. Flattening the structure and allowing locals to be used between modules would solve most problems I'm experiencing, though I guess there could be more complicated setups that might still need some nested, fancy shared variable hierarchy that your proposal seems to cover.

@yorinasub17
Copy link
Contributor Author

yorinasub17 commented Feb 12, 2020

UPDATE:

  • 542509f : Add walkthroughs of each scenario for read_terragrunt_config. Note that after walking through the examples, the only way I see this avoiding the verbosity is if we convert all the blocks to attributes.

  • 2b1039d : Include commentary on how RFC: single terragrunt.hcl per environment? #759 addresses the details in this RFC.

Copy link
Member

@brikis98 brikis98 left a comment

Thanks for updating the RFC with details from that new proposal! I left a few thoughts/comments.

See also #759 (comment) for a thought I had...

To understand this, consider a use case where you are operating in two regions, `us-east-1` and `eu-west-1`. To simplify
this example, consider a limited application deployment where you have two modules: `vpc` and `app`.

If we assume that `locals` are scoped within the file that they are defined, then you can implement this by separating
Copy link
Member

@brikis98 brikis98 Feb 12, 2020

Choose a reason for hiding this comment

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

Why would locals be scoped to a file? The idea with the new proposal is that all .hcl files in a single folder are processed as if they are in a single file, just as Terraform processes all .tf files in a single folder as if they are in a single file.

Copy link
Contributor Author

@yorinasub17 yorinasub17 Feb 12, 2020

Choose a reason for hiding this comment

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

There is an advantage to having vars scoped to the file so that you don't have to keep picking new names for a thing like region.

Copy link
Member

@brikis98 brikis98 Feb 12, 2020

Choose a reason for hiding this comment

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

Agreed, but I think in net, treating all .hcl files in a folder as if they were in one .hcl file, just as Terraform does with .tf files, is the better trade-off.

Copy link
Contributor Author

@yorinasub17 yorinasub17 Feb 12, 2020

Choose a reason for hiding this comment

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

I'll update to note that it is preferred to have scope of locals shared, but mention the possibility of having file scoped vars, its advantages and disadvantages.

```

However, if the namespace of `locals` was shared across the environment, then the above approach would not work and each
region file will need to define a different name for the region variable.
Copy link
Member

@brikis98 brikis98 Feb 12, 2020

Choose a reason for hiding this comment

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

Yea, if you're deploying the same things across regions, you'd use a different local for each region.


Reusing common variables depends on the scope of `locals`. If the scope of `locals` is shared across the environment,
then you can define the common variable in `locals` blocks to share across the entier environment. If instead the scope
of `locals` is per file, we either:
Copy link
Member

@brikis98 brikis98 Feb 12, 2020

Choose a reason for hiding this comment

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

Pretty sure locals and everything should be shared across all .hcl files in a folder, just like how Terraform does it for .tf files.

Copy link
Contributor Author

@yorinasub17 yorinasub17 Feb 12, 2020

Choose a reason for hiding this comment

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

See above for why one would want variables that are scoped to the file. E.g function scope vs global scope to avoid namespace clashes.

_docs/rfc/imports.md Outdated Show resolved Hide resolved
_docs/rfc/imports.md Outdated Show resolved Hide resolved
@jakauppila
Copy link
Contributor

jakauppila commented Feb 12, 2020

Looking at the suggestions, I'd be inclined to agree that import or read_terragrunt_config is the way way to go.

Our workaround for the for the differing inputs between configurations is to simply have two separate "top level" .hcl files that we include depending on the configuration so I think either solution would let us move away from that.

@brikis98 Flattening the structure as you proposed in #759 seems counter-intuitive to our use-case as I described in #759 (comment)

@jfunnell
Copy link

jfunnell commented Feb 12, 2020

Hm, I guess I can agree there is still possibly a need for imports even with a flattened structure, after seeing some examples.. Orthogonal enough to warrant implementing both, at least.

I think read_terragrunt_config is really simple and non-magical, and would compliment both the existing layout and a unified module approach.

@yorinasub17
Copy link
Contributor Author

yorinasub17 commented Feb 13, 2020

Thanks for the feedback everyone! I think based on the feedback and new thoughts around #759, it sounds to me like #759 is where we want to be heading. However, given how big of a change that really is, I am not sure that will get done anytime soon and, as @tomasbackman mentioned, there is a pressing need to solve this problem sooner rather than later.

Given that, I propose that we should take an incremental approach here, implementing the features that are low cost and partially solve the problem, and slowly work towards more complete solutions.

I think:

I'm going to start implementing read_terragrunt_config, but further feedback on that conclusion is very welcome!

EDIT: PR for read_terragrunt_config - #1051

@yorinasub17 yorinasub17 mentioned this pull request Feb 14, 2020
1 task
@brikis98
Copy link
Member

brikis98 commented Feb 14, 2020

Yea, the more I think about it, the more I believe the direction described in #759 (comment) is the way to go: turn Terragrunt into a preprocessor for more or less pure Terraform code. So many of the features we've built into Terragrunt already exist in Terraform (e.g., locals, helper functions, dependencies, etc), so many others become unnecessary with pure Terraform code (e.g., code generation of provider / backend blocks), and it does a better job of paving the way for Terraform to adopt these features we all want natively.

But yes, it'll take some time to do that, so +1 on putting in some reasonable stop gap solutions for now.

@yorinasub17
Copy link
Contributor Author

yorinasub17 commented Feb 27, 2020

UPDATE: I have updated the RFC to indicate that:

  • This is now in development
  • We will implement read_terragrunt_config, import blocks, and the single terragrunt.hcl file approach in that order.
  • Added relevant PR and release links

@yorinasub17
Copy link
Contributor Author

yorinasub17 commented Feb 27, 2020

@brikis98 any objections to merging this in now?

@brikis98
Copy link
Member

brikis98 commented Feb 27, 2020

No objection, go for it! Thx Yori!

@yorinasub17
Copy link
Contributor Author

yorinasub17 commented Feb 27, 2020

Thanks for sanity check! Going to merge it in now.

@yorinasub17 yorinasub17 merged commit caf88c9 into master Feb 27, 2020
@yorinasub17 yorinasub17 deleted the yori-enhanced-dependency branch Feb 27, 2020
@dudicoco
Copy link

dudicoco commented Mar 22, 2020

@yorinasub17 is there a PR open for the import blocks feature?

@yorinasub17
Copy link
Contributor Author

yorinasub17 commented Mar 26, 2020

Unfortunately, not yet.

@xcrezd
Copy link

xcrezd commented Jul 9, 2020

@yorinasub17 Any news on import feature?

@arash-bizcover
Copy link

arash-bizcover commented Jul 18, 2020

@yorinasub17 is there any way we can reference a hcl file from another git repo either with include, read_terragrunt_config or import
My use case is I don't want to keep some generic hcl files in the applications repos.

@yorinasub17
Copy link
Contributor Author

yorinasub17 commented Jul 18, 2020

We currently don't have plans to implement remote references for import, but a PR to add support for that is welcome.

@michelzanini
Copy link

michelzanini commented Sep 9, 2020

The import idea is brilliant. It will help many use cases we have.

I am not quite sure yet about #759. I would prefer to keep using the hierarchical folders. If you do get to implement #759 at some point, please make sure we can still do hierarchical folders. Mixing it all up in one single file should be optional.

As mentioned by @arash-bizcover , a remote import would be a great addition. We have repeated top level terragrunt.hcl files for multiple repositories. We need to keep syncing them in different repos. If we could import them all from a single Git repo it would be really good.

For example, consider something like this:

import "shared" {
  config_path = "git@github.com:my-company/shared-terragrunt.git/terragrunt.hcl?ref=master"
  merge = true
}

inputs = {
  
}

In this case config_path would support git references similar to the way source supports.

@jfunnell
Copy link

jfunnell commented Sep 15, 2020

I am not quite sure yet about #759. I would prefer to keep using the hierarchical folders. If you do get to implement #759 at some point, please make sure we can still do hierarchical folders. Mixing it all up in one single file should be optional.

It's not one file, it's as many as you want. You just dont need a subfolder per module. Please read the whole proposal because most people, including myself, have propogated this misinformation

@damon-atkins
Copy link

damon-atkins commented Dec 8, 2020

How would you implement the following if the functions are deprecated
key = "${basename(get_parent_terragrunt_dir())}/${path_relative_to_include()}/terraform.tfstate"

@yorinasub17
Copy link
Contributor Author

yorinasub17 commented Dec 8, 2020

How would you implement the following if the functions are deprecated

I seem to recall a conversation in one of the issues regarding path_relative_to_include in the almost 1 year that passed since I wrote this (wow time flies) where this turns out to be a critical feature for many users and most people prefer the current approach vs. the more explicit approach proposed here, so we will most likely keep this around until there is a better replacement.

As for get_parent_terragrunt_dir, the intention is to have get_terragrunt_dir return that path. That is, in import mode, get_terragrunt_dir will always return the dir of the config you wrote it in, not the dir of the importing config. We currently aren't thinking about introducing a replacement for get_importing_terragrunt_dir, but if there are enough use cases for it, we will most likely add it in.

Copy link

@Matausi29 Matausi29 left a comment

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