cycloidio / terracost

Cloud cost estimation for Terraform in your CLI
MIT License
278 stars 29 forks source link

terraform: estimation of HCL module directory #45

Closed patrislav closed 3 years ago

patrislav commented 3 years ago

This PR adds support for the estimation of HCL files in the form of a local Terraform module directory. It closes #29.

Limitations:

These limitations might be fine for the first version of the support.

patrislav commented 3 years ago

I'm not sure to be entirely able to review this PR as there are many pieces of code that I don't get. Which makes me think, it might be worth commenting more the function & also provide example of the data we are dealing with/extracting.

Yep, I tried to document it in the most important places but I will add some more. WDYM by the example? There is the Terraform module example in testdata, or do you mean something else?

Otherwise the set of tests are pretty good, I feel like it's missing a couple of those:

  1. Remote module(s) - all seem local ATM (could be a TODO)

There is no support for remote modules, only local at the moment. IMO for the first version, we can skip it and add it in later. Correct me if I'm wrong but I think all https://github.com/cycloid-community-catalog stacks use only local modules?

  1. Types seem simple - I don't see map/sliced but I'm not sure those have an impact cost wise?

Yes, at least for the resources we support there's only simple values or blocks (that are handled correctly.)

  1. There are no modules in modules (local nor remote)

Thanks, I will add this!

xlr-8 commented 3 years ago

There is no support for remote modules, only local at the moment. IMO for the first version, we can skip it and add it in later. Correct me if I'm wrong but I think all https://github.com/cycloid-community-catalog stacks use only local modules?

A vast majority of them indeed. But some do nevertheless: https://github.com/cycloid-community-catalog/stack-infrastructure/blob/master/terraform/module-infrastructure/vpc_infra.tf#L52-L54 or https://github.com/cycloid-community-catalog/stack-gke/blob/master/terraform/module-gke/control_plane.tf#L8-L11

So I agree we can still skip them for now - might be good if we are able to highlight it somewhere FE/BE.

WDYM by the example? There is the Terraform module example in testdata, or do you mean something else?

I mean within the codes comment to say the input is from X of the form map["resource"] or whatever we do these operations to return the object terraform.Module{["name"]: "resource} or whatever, but gives a bit more context on what operations are done on which type of data

patrislav commented 3 years ago

WDYM by the example? There is the Terraform module example in testdata, or do you mean something else?

I mean within the codes comment to say the input is from X of the form map["resource"] or whatever we do these operations to return the object terraform.Module{["name"]: "resource} or whatever, but gives a bit more context on what operations are done on which type of data

I added some more comments but if there are some places that I missed that would benefit from better doc, please mark them