Module depends_on

[jbardin]

Allow the use of depends_on for modules.

This PR stands on the existing work already done for module expansion, evaluation, and data source planning. See #24461, #24697, and #24904 for the majority of the work involved in making that happen.

Now that modules can be ordered and referenced as a whole, and data sources will cleanly allow forced ordering by depends_on; all that is left is to enable the feature in the config and add the graph connections for the depends_on references.

Closes #10462
Closes #17101

[codecov]

Codecov Report

Merging #25005 into master will decrease coverage by 0.00%.
The diff coverage is 70.00%.

Impacted Files	Coverage Δ
configs/module_call.go	67.56% <ø> (-2.44%)	⬇️
terraform/node_module_expand.go	87.50% <70.00%> (-1.79%)	⬇️
terraform/node_resource_plan.go	91.80% <0.00%> (-1.64%)	⬇️
terraform/evaluate.go	53.60% <0.00%> (+0.90%)	⬆️

[alisdair]

I might be missing something obvious, but I don't understand this comment. Why does re-planning after apply ensure that the data source isn't re-read?

[jbardin]

If the data source is read during plan (which is a new ability from #24904), it would show in the plan as a read operation. There should be nothing left to apply at this point, so anything in the plan should be NoOp.

[alisdair]

Ah, I see now, thanks!

[pselle]

I'm pulling this changeset down to put it through some paces on my machine, but in the meantime, looks like this doesn't include the docs if you want to add them here?

[apparentlymart]

I've not delved deep into this yet because I don't have the necessary context loaded to think about the details, but I wanted to ask: could we make sure there's a test somewhere for the case where module depends_on isn't being used and one of a module's variables depends on its outputs? I'd like to make sure that isn't broken (and stays unbroken, for the time being) for situations where users aren't using any of these new features.

The sort of test case I have in mind would be...

root module:

module "test" {
  source = "./test"

  a = module.test.b
}

output "c" {
  value = module.test.c
}

The ./test module:

variable "a" {}

resource "test" "test" {
}

output "b" {
  value = test.test.id
}

output "c" {
  value = var.a
}

I would expect the above to work and for the root module output c to be whatever test.test.id is in the child module. Based on an initial read of this code I expect this should still work, because without depends_on set in the module block there should not be any additional dependency edges created that would make this be a cycle.

We have so many test fixtures in Terraform Core that I wouldn't be surprised if something like this was there somewhere already, but I'm not sure where it is if so. 😕

[jbardin]

Good idea @apparentlymart! While I didn't expect that to be be effected by this PR, I want to make certain we have that test in the corpus.

[apparentlymart]

Having now had some extra time to familiarize myself with how the new "expand" nodes fit in with everything else 😄 this looks good to me! I did have a brief play with it to see how it looks and it matched what I was expecting, though I didn't have a deep play with it because @pselle already mentioned doing that. 😀

That this last change ended up being so straightforward and clear is, I think, a testament to all of the other refactoring work y'all have been doing these last few months. Good job! 🎉

[apparentlymart]

🙄 I forgot to select "Approve" when I submitted that, though with that said I don't mean to override @pselle's comment about the docs. Hopefully it's a relatively straightforward docs update and so we could include that in here just to keep this change "atomic".

[jbardin]

The docs change is minimal I think. I just have that in another PR since data sources also needed a depends_on update, and then Nick could also review the docs separately.

[ozbillwang]

I think this would be a big change (I mean a lot of codes need be updated), but not.

That's amazing, thanks a lot.

[feliperfmarques]

Thanks a lot for this feature. Looking forward to this release.

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.