hashicorp / terraform-cdk

Define infrastructure resources using programming constructs and provision them using HashiCorp Terraform
https://www.terraform.io/cdktf
Mozilla Public License 2.0
4.87k stars 455 forks source link

Documentation: Code in `stacks.mdx` sourced from documentation examples but not actually verified #2579

Closed ansgarm closed 9 months ago

ansgarm commented 1 year ago

Community Note

Description

The following code is sourced from documentation examples: https://github.com/hashicorp/terraform-cdk/blob/b3c34a80e75a167b758807bcebfebd06d360c168/website/docs/cdktf/concepts/stacks.mdx?plain=1#L752-L762

But it's coming from a commented out section for some: https://github.com/hashicorp/terraform-cdk/blob/b3c34a80e75a167b758807bcebfebd06d360c168/examples/python/documentation/stacks.py#L135-L139 https://github.com/hashicorp/terraform-cdk/blob/b3c34a80e75a167b758807bcebfebd06d360c168/examples/csharp/documentation/CrossStackReferenceStack.cs#L96-L100

Which defeats the purpose of having these snippets in examples in the first place as that code is not verified to be correct, and in the case of CSharp it sadly also isn't correct (Fn.MergeLists would be right).

Opposed to these short snippets the TypeScript example is a longer one that tries to give a full example for using a local for transitive dependencies in stacks. When converting this snippet to Go, I tried to improve it one step further and actually call my stacks A, B, C, and D to match the explanation above the snippets:

If you e.g. write this.allResources = Fn.mergeLists(resourceFromStackA.items, resourceFromStackB.items) in Stack C and use stackC.allResources in Stack D, Stack D will be dependant on Stack A and B, but not C since that is not the origin of the data.

Go example (from PR #2569): https://github.com/hashicorp/terraform-cdk/commit/572005b52697c1b956704758a0c490ca31e38fe2#diff-a4eea5a9e601e457f62577f95db14bef8aa11614f97ce7e667fdb389404acf14R935-R1009

My recommendation would be to adjust the other examples to match the Go example but that's said easy as I wrote it, so I'm open to discuss if there's a better way to explain how to use locals to circumvent "inlining" transitive dependencies.

References

ansgarm commented 1 year ago

To extend on this (and not create an extra issue that might be superseded by solving this): The TypeScript example specifies a dependencies argument but does not actually use it: https://github.com/hashicorp/terraform-cdk/blob/29538f8a875769f6ba9de5e00e7a1cd10ef18d7a/examples/typescript/documentation/stacks.ts#L165

Instead it accesses the stacks sourceStackA and sourceStackB directly from the root context of that file which seems like a code smell.

github-actions[bot] commented 8 months ago

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've 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.