Implement data aws_pricing_product #5057

julienduchesne · 2018-07-03T11:53:31Z

Changes proposed in this pull request:

Adds a aws_pricing_product data
filters are generic and are passed directly to the API (https://docs.aws.amazon.com/aws-cost-management/latest/APIReference/API_pricing_GetProducts.html)
AWS returns a large json result. To parse through the result, I added the gjson library (https://github.com/tidwall/gjson), this is accessible through the json_query attribute. I understand that you may want to reduce dependencies but filtering through json results is actually quite complex and I have not seen anywhere else in the provider where it is done. I am open to suggestions on this.

Output from acceptance testing:

make testacc TESTARGS='-run=TestAccDataSourceAwsPricingProduct'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run=TestAccDataSourceAwsPricingProduct -timeout 120m
?       github.com/terraform-providers/terraform-provider-aws   [no test files]
=== RUN   TestAccDataSourceAwsPricingProduct_ec2
--- PASS: TestAccDataSourceAwsPricingProduct_ec2 (12.48s)
=== RUN   TestAccDataSourceAwsPricingProduct_redshift
--- PASS: TestAccDataSourceAwsPricingProduct_redshift (11.98s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       24.467s

bflad · 2018-07-03T14:11:45Z

Hi @julienduchesne, thanks for submitting this data source! I did want to touch base briefly about the below:

AWS returns a large json result. To parse through the result, I added the gjson library (https://github.com/tidwall/gjson), this is accessible through the json_query attribute. I understand that you may want to reduce dependencies but filtering through json results is actually quite complex and I have not seen anywhere else in the provider where it is done. I am open to suggestions on this.

We are just starting the process of writing up Terraform provider development best practices documentation and this is likely an item that would appear there. 😄

At first blush, this seems like it belongs in its own separate data source and most likely in its own provider, rather than something to implement piecemeal per resource and data source everywhere. For example, from an operator perspective, this might be useful (calling the new provider json and the data source json_query for illustrative purposes):

data "aws_pricing_product" "example" {
  service_code = "AmazonRedshift"

  filters = [
    {
      field = "instanceType"
      value = "ds1.xlarge"
    },
    {
      field = "location"
      value = "US East (N. Virginia)"
    },
  ]
}

data "json_query" "example" {
  json = "${data.aws_pricing_product.example.json}"
  query = "terms.OnDemand.*.priceDimensions.*.pricePerUnit.USD"
}

output "example" {
  values = "${data.json_query.example.value}"
}

I cannot guarantee that HashiCorp would necessarily be willing to maintain this as a new provider, but if it makes sense from a generic sense, I can reach out and gather opinions on hosting it as a community provider as part of the Terraform Provider Development Program.

There is also an existing community provider implementation that might be able to help here: https://github.com/EvilSuperstars/terraform-provider-jsondecode

Better news though is that while the Terraform 0.12 preview blog post did not specifically mention it, Terraform 0.12 will be able to support a jsondecode() built-in function due to the strong typing being implemented throughout Terraform core. In that case, I believe it should be possible to do something like:

data "aws_pricing_product" "example" {
  service_code = "AmazonRedshift"

  filters = [
    {
      field = "instanceType"
      value = "ds1.xlarge"
    },
    {
      field = "location"
      value = "US East (N. Virginia)"
    },
  ]
}

# Potential Terraform 0.12 syntax - may change during implementation
# Also, not sure about the exact attribute reference architecture myself :)
output "example" {
  values = jsondecode(data.json_query.example.value).terms.OnDemand.*.priceDimensions.*.pricePerUnit.USD
}

That feature can be tracked upstream at: hashicorp/terraform#10363

With all that said, we likely would not want to merge in the extra dependency or json_query handling in this data source as it is better suited with a more generic solution. If you can remove those out of this pull request, that will greatly speed up getting this merged. 👍

julienduchesne · 2018-07-03T22:57:56Z

Alright, here you go

make testacc TESTARGS='-run=TestAccDataSourceAwsPricingProduct'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run=TestAccDataSourceAwsPricingProduct -timeout 120m
?       github.com/terraform-providers/terraform-provider-aws   [no test files]
=== RUN   TestAccDataSourceAwsPricingProduct_ec2
--- PASS: TestAccDataSourceAwsPricingProduct_ec2 (14.45s)
=== RUN   TestAccDataSourceAwsPricingProduct_redshift
--- PASS: TestAccDataSourceAwsPricingProduct_redshift (14.05s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       28.508s

Let me know if anything else is not OK

bflad

This is looking pretty good 👍 A few minor things below and this should be ready to ship.

bflad · 2018-07-04T00:10:24Z

aws/data_source_aws_pricing_product.go

+package aws
+
+import (
+	"log"


Minor nitpick: The Go imports seem to be a little off formatting wise. The stdlib imports should be grouped and alphabetically sorted at the top, a blank line, then external imports grouped and alphabetically sorted.

We tend to prefer using goimports for managing these consistently (and automatically) everywhere 👍

bflad · 2018-07-04T00:11:34Z

aws/data_source_aws_pricing_product.go

+)
+
+const (
+	awsPricingTermMatch = "TERM_MATCH"


We should prefer to use the available SDK constant: pricing.FilterTypeTermMatch

bflad · 2018-07-04T01:14:10Z

aws/data_source_aws_pricing_product.go

+		return fmt.Errorf("Error reading pricing of products: %s", err)
+	}
+
+	if err = verifyProductsPriceListLength(resp.PriceList); err != nil {


Minor nitpick: given this function is not being used elsewhere and not explicitly unit tested, we can remove the indirection and just include the logic inline here.

bflad · 2018-07-04T01:39:06Z

aws/data_source_aws_pricing_product_test.go

+)
+
+func TestAccDataSourceAwsPricingProduct_ec2(t *testing.T) {
+	os.Setenv("AWS_DEFAULT_REGION", "us-east-1")


We should save the original value and reset it after completion or this could break other tests:

oldRegion := os.Getenv("AWS_DEFAULT_REGION") os.Setenv("AWS_DEFAULT_REGION", "us-east-1") defer os.Setenv("AWS_DEFAULT_REGION", oldRegion)

The same should be applied to the other acceptance test. Admittedly, we need to remove using environment variables for this purpose. 🙁

bflad · 2018-07-04T01:44:24Z

website/docs/d/pricing_product.html.markdown

+## Argument Reference
+
+ * `service_code` - (Required) The code of the service. Available service codes can be fetched using the DescribeServices pricing API call.
+ * `filters` - (Required) A list of filters. Passed directly to the API (see GetProducts API reference). These filters must describe a single product, this resource will fail if more than one product is returned by the API.


We need to describe the filters nested fields as well, e.g.

### filters * `field` (Required) The attribute name that you want to filter on. * `value` (Required) The attribute value that you want to filter on.

bflad · 2018-07-04T01:48:09Z

website/docs/d/pricing_product.html.markdown

+---
+layout: "aws"
+page_title: "AWS: aws_pricing_product"
+sidebar_current: "docs-aws-datasource-pricing-product"


This page needs a sidebar link added to website/aws.erb

bflad · 2018-07-04T02:20:14Z

Looks great, @julienduchesne! 🚀

make testacc TEST=./aws TESTARGS='-run=TestAccDataSourceAwsPricingProduct'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccDataSourceAwsPricingProduct -timeout 120m
=== RUN   TestAccDataSourceAwsPricingProduct_ec2
--- PASS: TestAccDataSourceAwsPricingProduct_ec2 (6.03s)
=== RUN   TestAccDataSourceAwsPricingProduct_redshift
--- PASS: TestAccDataSourceAwsPricingProduct_redshift (4.86s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	10.928s

bflad · 2018-07-04T12:31:20Z

This has been released in version 1.26.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

tomelliff · 2020-02-10T11:59:19Z

I couldn't quite get the pre 0.12 proposed syntax working. The following seems to work okay though:

variable "instance_type" {}

provider "aws" {
  alias  = "us-east-1"
  region = "us-east-1"
}

data "aws_region" "current" {}

data "aws_pricing_product" "example" {
  provider = aws.us-east-1

  service_code = "AmazonEC2"

  filters {
    field = "capacitystatus"
    value = "Used"
  }

  filters {
    field = "instanceType"
    value = var.instance_type
  }

  filters {
    field = "operatingSystem"
    value = "Linux"
  }

  filters {
    field = "location"
    value = data.aws_region.current.description
  }

  filters {
    field = "preInstalledSw"
    value = "NA"
  }

  filters {
    field = "licenseModel"
    value = "No License required"
  }

  filters {
    field = "tenancy"
    value = "Shared"
  }
}

output "on_demand_price" {
  value = tonumber(values(values(jsondecode(data.aws_pricing_product.example.result).terms.OnDemand)[0].priceDimensions)[0].pricePerUnit.USD)
}

Not sure if there's a better/cleaner way to be getting at the relevant data though?

Would it be worth adding this as an example to the resource's documentation?

ghost · 2020-02-10T14:50:19Z

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

Julien Duchesne added 2 commits July 2, 2018 18:54

Implement aws pricing data source

fff24f7

Added tests for aws_pricing_product

d3e1cd5

ghost added size/XXL Managed by automation to categorize the size of a PR. dependencies Used to indicate dependency changes. labels Jul 3, 2018

Add aws_pricing_product doc

68d64c1

julienduchesne force-pushed the aws-pricing branch from 1711f4d to 68d64c1 Compare July 3, 2018 12:01

ghost added size/XXL Managed by automation to categorize the size of a PR. dependencies Used to indicate dependency changes. labels Jul 3, 2018

bflad added the service/pricing Issues and PRs that pertain to the pricing service. label Jul 3, 2018

bflad added waiting-response Maintainers are waiting on response from community or contributor. new-data-source Introduces a new data source. labels Jul 3, 2018

ghost added size/XL Managed by automation to categorize the size of a PR. dependencies Used to indicate dependency changes. and removed size/XXL Managed by automation to categorize the size of a PR. labels Jul 3, 2018

Remove JSON dependency according to PR comments

d403191

julienduchesne force-pushed the aws-pricing branch from 88667be to d403191 Compare July 3, 2018 22:55

ghost added size/L Managed by automation to categorize the size of a PR. and removed size/XL Managed by automation to categorize the size of a PR. dependencies Used to indicate dependency changes. labels Jul 3, 2018

bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label Jul 4, 2018

bflad reviewed Jul 4, 2018

View reviewed changes

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Jul 4, 2018

ghost added the size/L Managed by automation to categorize the size of a PR. label Jul 4, 2018

Fix comments from PR hashicorp#5057 (pricing_product)

1d729c1

julienduchesne force-pushed the aws-pricing branch from 22a621c to 1d729c1 Compare July 4, 2018 02:14

ghost added the size/L Managed by automation to categorize the size of a PR. label Jul 4, 2018

bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label Jul 4, 2018

bflad added this to the v1.26.0 milestone Jul 4, 2018

bflad merged commit 1d729c1 into hashicorp:master Jul 4, 2018

bflad mentioned this pull request Jul 4, 2018

data aws_pricing_instance #5009

Closed

bflad added a commit that referenced this pull request Jul 4, 2018

Update CHANGELOG for #5057

35c0728

julienduchesne deleted the aws-pricing branch July 4, 2018 10:14

ghost locked and limited conversation to collaborators Feb 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement data aws_pricing_product #5057

Implement data aws_pricing_product #5057

julienduchesne commented Jul 3, 2018

bflad commented Jul 3, 2018

julienduchesne commented Jul 3, 2018 •

edited

Loading

bflad left a comment

bflad Jul 4, 2018

bflad Jul 4, 2018

bflad Jul 4, 2018

bflad Jul 4, 2018

bflad Jul 4, 2018

bflad Jul 4, 2018

bflad commented Jul 4, 2018

bflad commented Jul 4, 2018

tomelliff commented Feb 10, 2020

ghost commented Feb 10, 2020

Implement data aws_pricing_product #5057

Implement data aws_pricing_product #5057

Conversation

julienduchesne commented Jul 3, 2018

bflad commented Jul 3, 2018

julienduchesne commented Jul 3, 2018 • edited Loading

bflad left a comment

Choose a reason for hiding this comment

bflad Jul 4, 2018

Choose a reason for hiding this comment

bflad Jul 4, 2018

Choose a reason for hiding this comment

bflad Jul 4, 2018

Choose a reason for hiding this comment

bflad Jul 4, 2018

Choose a reason for hiding this comment

bflad Jul 4, 2018

Choose a reason for hiding this comment

bflad Jul 4, 2018

Choose a reason for hiding this comment

bflad commented Jul 4, 2018

bflad commented Jul 4, 2018

tomelliff commented Feb 10, 2020

ghost commented Feb 10, 2020

julienduchesne commented Jul 3, 2018 •

edited

Loading