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

feat: add monthly_hrs usage field for aws/azure/gcp VMs #1812

Merged

Conversation

jessecureton
Copy link
Contributor

@jessecureton jessecureton commented Jul 4, 2022

Updates the following resource types to support a monthly_hrs usage flag:

  • aws_instance
  • azurerm_virtual_machine
  • azurerm_linux_virtual_machine
  • azurerm_windows_virtual_machine
  • google_compute_instance

This addresses #1556.

@CLAassistant
Copy link

CLAassistant commented Jul 4, 2022

CLA assistant check
All committers have signed the CLA.

@jessecureton
Copy link
Contributor Author

At its core this is a pretty straightforward PR, but the myriad golden file updates have made it appear larger than it is.

Happy to break this up into a separate PR each for AWS / Azure / GCP resources, if the maintainers prefer. Looking forward to getting feedback and getting this landed!

@jessecureton jessecureton changed the title feat: add support for monthly_hours usage field for aws/azure/gcp VMs feat: add monthly_hours usage field for aws/azure/gcp VMs Jul 4, 2022
@aliscott
Copy link
Member

aliscott commented Jul 5, 2022

Thanks for the contribution @jessecureton 🙏! One of our team will look at this in the next couple of days.

@jessecureton
Copy link
Contributor Author

Sounds good! I took a look at the failing test case in the CI checks and it seems unrelated to this change. I did notice the failing test case (TestBreakdownWithPrivateTerraformRegistryModule) does not run locally, and instead prints the following message.

breakdown_test.go:369: Skipping because INFRACOST_TERRAFORM_CLOUD_TOKEN is not set and external contributors won't have this.

Locally running make test_all passes all tests as expected, so hazarding a guess this seems like it might be something related to CI secrets when running in my forked repo?

@aliscott
Copy link
Member

aliscott commented Jul 5, 2022

@jessecureton you're correct. I pushed a fix to skip this test in #1809, but I think there might still be issues here I haven't had a chance to investigate today.

Copy link
Member

@aliscott aliscott left a comment

Choose a reason for hiding this comment

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

Thank you @jessecureton, this looks great 🙂. Two minor points below.

  1. I think we should we use monthly_hrs to be consistent with the other *_hrs usage keys.
  2. Should it be a float value for the use-case if a user only runs the instance for a few minutes in the month?

@jessecureton
Copy link
Contributor Author

1. I think we should we use `monthly_hrs` to be consistent with the other `*_hrs` usage keys.

I used monthly_hours to match the same field name already in use in several other resources - namely azurerm_synapse_spark_pool, aws_glue_job, and aws_glue_crawler. That said, I'm happy to make this change if you still prefer the _hrs syntax, just wanted to point out the mixed convention already in use.

2. Should it be a float value for the use-case if a user only runs the instance for a few minutes in the month?

This seems reasonable! I will make this change alongside the above once I hear back re: the preferred syntax

@aliscott
Copy link
Member

aliscott commented Jul 7, 2022

I used monthly_hours to match the same field name already in use in several other resources - namely azurerm_synapse_spark_pool, aws_glue_job, and aws_glue_crawler. That said, I'm happy to make this change if you still prefer the _hrs syntax, just wanted to point out the mixed convention already in use.

Yeah we definitely have inconsistencies at the moment 😞 and at some point we should migrate those to be consistent. For now we should use _hrs to be consistent with the resource mapping guidelines.

@jessecureton
Copy link
Contributor Author

I've updated this to include both the requested changes for float values and the switch to monthly_hrs! Thanks for the link to the policy doc for naming, I had missed that initially!

@jessecureton jessecureton changed the title feat: add monthly_hours usage field for aws/azure/gcp VMs feat: add monthly_hrs usage field for aws/azure/gcp VMs Jul 10, 2022
@aliscott aliscott self-requested a review July 10, 2022 15:55
Copy link
Member

@aliscott aliscott left a comment

Choose a reason for hiding this comment

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

Thanks @jessecureton, this looks great! I'll rebase and run the tests, to make sure they all pass.

@aliscott aliscott force-pushed the jcureton/add-monthly-hours-for-vms branch from a207a1f to 36f8ca1 Compare July 11, 2022 17:23
@aliscott aliscott merged commit d7582f6 into infracost:master Jul 11, 2022
rshade pushed a commit to rshade/infracost that referenced this pull request Jul 18, 2022
* first pass at ec2 instance monthly hours

* add monthly hour usage example to golden files

* handle usage quantities for other aws cost components

* update goldens

* remove unsupported elastic inference accelerator test cases

* support monthly_hours on azure vm resources

* add google cloud compute instance hourly usage

* fix a missed bit of formatting

* update usage golden files and add makefile aliases for them

* update usage field names to use _hrs

* use floats to allow decimal monthly_hrs values
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

3 participants