[go: nahoru, domu]
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.

Sign up for GitHub

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

Jump to bottom

terraform: estimation of HCL module directory #45

Merged
merged 5 commits into from
May 31, 2021
Merged

terraform: estimation of HCL module directory #45

merged 5 commits into from
May 31, 2021

Conversation

patrislav
Copy link
Contributor

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

Limitations:

  • No support for including other types of modules (e.g. registry, git, etc.) - only local module paths will work
  • Only the "planned" part of the plan is estimated, leaving the "prior" empty - this could be solved only by support for tfstate (Support Terraform state files (tfstate) #28)

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

@patrislav patrislav self-assigned this May 19, 2021
xlr-8
xlr-8 reviewed May 20, 2021
View reviewed changes
Copy link
Contributor
@xlr-8 xlr-8 left a comment

Choose a reason for hiding this comment

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

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.

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)
  2. Types seem simple - I don't see map/sliced but I'm not sure those have an impact cost wise?
  3. There are no modules in modules (local nor remote)

terraform/hcl.go
if rv.ProviderConfigRef != nil {
providerKey = rv.ProviderConfigRef.String()
}
provider := providers[providerKey]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can only be found I guess?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since it's an interface{}, it will be nil if not found in the map. This is then checked later down: https://github.com/cycloidio/terracost/pull/45/files/5583ecf7da89196e5d59e1c64f1d5838d13148e3..4e0d5949fce9e0218d1f2b9f0f9184dc967e4952#diff-d88d3b92409f1211e4b636160025511bac9b8ab47d7c06375fd0092f53cf5763R69

terraform/hcl.go
if !val.IsKnown() {
continue
}
switch val.Type() {
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to deal with maps or slices?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO there's no need, I can't think of any resource that would use them for any cost-related attributes. For sure we don't support any ATM. Though there's bool missing that I will add.

@patrislav
Copy link
Contributor Author

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
Copy link
Contributor
xlr-8 commented May 21, 2021 •
edited
Loading

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 patrislav marked this pull request as ready for review May 27, 2021 13:34
@patrislav
Copy link
Contributor Author

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

xlr-8
xlr-8 previously approved these changes May 31, 2021
View reviewed changes
Patryk Kalinowski added 4 commits May 31, 2021 17:34
terraform: add ExtractQueriesFromHCL function
ba0d9c7 
The new functionality includes extraction of resources from HCL modules
in the form of local directories.
testdata: add a test Terraform module
e70bd07
terracost: add EstimateHCL function
b18cbd0 
The EstimateHCL function estimates the plan costs based on an HCL module
directory.
tests: add tests for the EstimateHCL function
6ad4a2c
@patrislav patrislav merged commit 09fd63e into master May 31, 2021
@patrislav patrislav deleted the pk-hcl-parsing branch May 31, 2021 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xlr-8 xlr-8 xlr-8 left review comments

Assignees

@patrislav patrislav

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support estimation of a directory of HCL files
2 participants
@patrislav @xlr-8