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

aws_cur_report_definition - Parquet, Athena and more #12428

Merged
merged 13 commits into from
Sep 2, 2020
Merged

aws_cur_report_definition - Parquet, Athena and more #12428

merged 13 commits into from
Sep 2, 2020

Conversation

robbruce
Copy link
Contributor

This change updates aws_cur_report_definition resource and data source to match options available on AWS.

Added:

  • Parquet Format
  • Athena Additional Artifact
  • Refresh Closed Reports
  • Report Versioning

When using the AWS cur API, it very unhelpfully returns an empty ValidationException when certain combinations of options conflict with each other. The allowed combination of options has been added as part of the create step. For example, if Athena is being used the output format and compression must be Parquet and the s3 prefix cannot be empty!

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Relates #8765

Release note for CHANGELOG:

Add Parquet Format, Athena Additional Artifact, Refresh Closed Reports and Report Versioning to aws_cur_report_definition resource and data source

Some slight changes were made on the acceptance tests because some race conditions can occur when the bucket policy had not finished applying (even with a depends_on) and so a ValidationException occurs.

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAwsCurReportDefinition'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -count 1 -parallel 20 -run=TestAccAwsCurReportDefinition -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAwsCurReportDefinition_basic
=== PAUSE TestAccAwsCurReportDefinition_basic
=== RUN   TestAccAwsCurReportDefinition_parquet
=== PAUSE TestAccAwsCurReportDefinition_parquet
=== RUN   TestAccAwsCurReportDefinition_athena
=== PAUSE TestAccAwsCurReportDefinition_athena
=== RUN   TestAccAwsCurReportDefinition_refresh
=== PAUSE TestAccAwsCurReportDefinition_refresh
=== RUN   TestAccAwsCurReportDefinition_overwrite
=== PAUSE TestAccAwsCurReportDefinition_overwrite
=== CONT  TestAccAwsCurReportDefinition_basic
=== CONT  TestAccAwsCurReportDefinition_refresh
=== CONT  TestAccAwsCurReportDefinition_parquet
=== CONT  TestAccAwsCurReportDefinition_overwrite
=== CONT  TestAccAwsCurReportDefinition_athena
--- PASS: TestAccAwsCurReportDefinition_overwrite (80.17s)
--- PASS: TestAccAwsCurReportDefinition_basic (80.65s)
--- PASS: TestAccAwsCurReportDefinition_refresh (80.78s)
--- PASS: TestAccAwsCurReportDefinition_parquet (80.90s)
--- PASS: TestAccAwsCurReportDefinition_athena (83.89s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	85.611s
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws/internal/flatmap	0.560s [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags	0.268s [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws/internal/naming	0.532s [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws/internal/service/batch/equivalency	0.179s [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws/internal/service/eks/token	0.250s [no tests to run]
?   	github.com/terraform-providers/terraform-provider-aws/awsproviderlint	[no test files]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/awsproviderlint/passes	0.495s [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/awsproviderlint/passes/AWSAT001	0.350s [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/awsproviderlint/passes/AWSR001	0.150s [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/awsproviderlint/passes/fmtsprintfcallexpr	0.106s [no tests to run]
$ make testacc TESTARGS='-run=TestAccDataSourceAwsCurReportDefinition'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -count 1 -parallel 20 -run=TestAccDataSourceAwsCurReportDefinition -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccDataSourceAwsCurReportDefinition_basic
=== PAUSE TestAccDataSourceAwsCurReportDefinition_basic
=== CONT  TestAccDataSourceAwsCurReportDefinition_basic
--- PASS: TestAccDataSourceAwsCurReportDefinition_basic (89.42s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	91.195s
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws/internal/flatmap	0.657s [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags	0.342s [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws/internal/naming	0.423s [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws/internal/service/batch/equivalency	0.164s [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws/internal/service/eks/token	0.259s [no tests to run]
?   	github.com/terraform-providers/terraform-provider-aws/awsproviderlint	[no test files]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/awsproviderlint/passes	0.370s [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/awsproviderlint/passes/AWSAT001	0.457s [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/awsproviderlint/passes/AWSR001	0.126s [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/awsproviderlint/passes/fmtsprintfcallexpr	0.229s [no tests to run]

@ghost ghost added needs-triage Waiting for first response or review from a maintainer. size/XL Managed by automation to categorize the size of a PR. service/costandusagereportservice tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. documentation Introduces or discusses updates to documentation. labels Mar 17, 2020
@robbruce robbruce changed the title [WIP] aws_cur_report_definition - Parquet, Athena and more aws_cur_report_definition - Parquet, Athena and more Mar 17, 2020
@robbruce robbruce marked this pull request as ready for review March 17, 2020 15:48
@robbruce robbruce requested a review from a team March 17, 2020 15:48
@robbruce
Copy link
Contributor Author

Very similar to #11567, but includes refresh and version options as well as data resource.

Didn't know 11567 existed until after creating the pull request!

@andymoore-synamedia
Copy link

currently using this in a stage env to test (in the hope this gets merged some time in the future and we can go back to proper master...)

hit a couple of things when trying to use, might be worth adding them to the docs:

When ATHENA exists within additional_artifacts, no other artifact type can be declared
When ATHENA exists within additional_artifacts, report_versioning must be OVERWRITE_REPORT```

@andymoore-synamedia
Copy link

👍

@sckevmit
Copy link

Can we please get this merged?

@robbruce
Copy link
Contributor Author

Updated and re-ran acceptance tests:

make testacc TESTARGS='-run=TestAccAwsCurReportDefinition'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAwsCurReportDefinition -timeout 120m
=== RUN   TestAccAwsCurReportDefinition_basic
=== PAUSE TestAccAwsCurReportDefinition_basic
=== RUN   TestAccAwsCurReportDefinition_parquet
=== PAUSE TestAccAwsCurReportDefinition_parquet
=== RUN   TestAccAwsCurReportDefinition_athena
=== PAUSE TestAccAwsCurReportDefinition_athena
=== RUN   TestAccAwsCurReportDefinition_refresh
=== PAUSE TestAccAwsCurReportDefinition_refresh
=== RUN   TestAccAwsCurReportDefinition_overwrite
=== PAUSE TestAccAwsCurReportDefinition_overwrite
=== CONT  TestAccAwsCurReportDefinition_basic
=== CONT  TestAccAwsCurReportDefinition_athena
=== CONT  TestAccAwsCurReportDefinition_refresh
=== CONT  TestAccAwsCurReportDefinition_overwrite
=== CONT  TestAccAwsCurReportDefinition_parquet
--- PASS: TestAccAwsCurReportDefinition_refresh (38.38s)
--- PASS: TestAccAwsCurReportDefinition_overwrite (38.89s)
--- PASS: TestAccAwsCurReportDefinition_athena (38.90s)
--- PASS: TestAccAwsCurReportDefinition_parquet (38.92s)
--- PASS: TestAccAwsCurReportDefinition_basic (39.53s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	41.293s
make testacc TESTARGS='-run=TestAccDataSourceAwsCurReportDefinition'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccDataSourceAwsCurReportDefinition -timeout 120m
=== RUN   TestAccDataSourceAwsCurReportDefinition_basic
=== PAUSE TestAccDataSourceAwsCurReportDefinition_basic
=== CONT  TestAccDataSourceAwsCurReportDefinition_basic
--- PASS: TestAccDataSourceAwsCurReportDefinition_basic (43.24s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	44.900s

@ewbankkit
Copy link
Contributor

ewbankkit commented Aug 26, 2020

@robbruce Thanks for the contribution.
A couple of (relatively minor) suggestions:

  • Replace explicit list of values in the ValidateFuncs with AWS SDK _Values(): Regression between 0.12.24 and 0.12.29 on ec2metadata calls #14624.
  • Could you split the acceptance test configuration generating function to keep _basic as it was and add a second one that takes all the new options you added? This will help ensure that there are no regressions.

@robbruce
Copy link
Contributor Author

@ewbankkit changes requested have been implemented.

Note, this also fixes #14827 (a bug introduced by #14432)

Unit tests re-ran (had to reduce parallel tests as an AWS limit was reached on our account as other reports already existed!)

make testacc TESTARGS='-run=TestAccAwsCurReportDefinition' ACCTEST_PARALLELISM=1                                                                                                                                         
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 1 -run=TestAccAwsCurReportDefinition -timeout 120m
=== RUN   TestAccAwsCurReportDefinition_basic
=== PAUSE TestAccAwsCurReportDefinition_basic
=== RUN   TestAccAwsCurReportDefinition_textOrCsv
=== PAUSE TestAccAwsCurReportDefinition_textOrCsv
=== RUN   TestAccAwsCurReportDefinition_parquet
=== PAUSE TestAccAwsCurReportDefinition_parquet
=== RUN   TestAccAwsCurReportDefinition_athena
=== PAUSE TestAccAwsCurReportDefinition_athena
=== RUN   TestAccAwsCurReportDefinition_refresh
=== PAUSE TestAccAwsCurReportDefinition_refresh
=== RUN   TestAccAwsCurReportDefinition_overwrite
=== PAUSE TestAccAwsCurReportDefinition_overwrite
=== CONT  TestAccAwsCurReportDefinition_basic
--- PASS: TestAccAwsCurReportDefinition_basic (47.10s)
=== CONT  TestAccAwsCurReportDefinition_refresh
--- PASS: TestAccAwsCurReportDefinition_refresh (45.45s)
=== CONT  TestAccAwsCurReportDefinition_overwrite
--- PASS: TestAccAwsCurReportDefinition_overwrite (45.52s)
=== CONT  TestAccAwsCurReportDefinition_parquet
--- PASS: TestAccAwsCurReportDefinition_parquet (47.17s)
=== CONT  TestAccAwsCurReportDefinition_athena
--- PASS: TestAccAwsCurReportDefinition_athena (45.53s)
=== CONT  TestAccAwsCurReportDefinition_textOrCsv
--- PASS: TestAccAwsCurReportDefinition_textOrCsv (47.58s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	280.045s
make testacc TESTARGS='-run=TestAccDataSourceAwsCurReportDefinition' ACCTEST_PARALLELISM=1                                                                                                                               
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 1 -run=TestAccDataSourceAwsCurReportDefinition -timeout 120m
=== RUN   TestAccDataSourceAwsCurReportDefinition_basic
=== PAUSE TestAccDataSourceAwsCurReportDefinition_basic
=== RUN   TestAccDataSourceAwsCurReportDefinition_additional
=== PAUSE TestAccDataSourceAwsCurReportDefinition_additional
=== CONT  TestAccDataSourceAwsCurReportDefinition_basic
--- PASS: TestAccDataSourceAwsCurReportDefinition_basic (88.86s)
=== CONT  TestAccDataSourceAwsCurReportDefinition_additional
--- PASS: TestAccDataSourceAwsCurReportDefinition_additional (137.71s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	228.578s

Copy link
Contributor

@ewbankkit ewbankkit left a comment

Choose a reason for hiding this comment

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

LGTM.

--- PASS: TestAccDataSourceAwsCurReportDefinition_additional (14.50s)
--- PASS: TestAccDataSourceAwsCurReportDefinition_basic (14.71s)
--- PASS: TestAccAwsCurReportDefinition_basic (15.66s)
--- PASS: TestAccAwsCurReportDefinition_refresh (15.66s)
--- PASS: TestAccAwsCurReportDefinition_parquet (14.35s)
--- PASS: TestAccAwsCurReportDefinition_athena (14.13s)
--- PASS: TestAccAwsCurReportDefinition_textOrCsv (13.74s)
--- PASS: TestAccAwsCurReportDefinition_overwrite (14.16s)

Comment on lines 125 to 194
hasAthena := false

// perform various combination checks, AWS API unhelpfully just returns an empty ValidationException
// these combinations have been determined from the Create Report AWS Console Web Form
for _, artifact := range additionalArtifacts {
if *artifact == costandusagereportservice.AdditionalArtifactAthena {
hasAthena = true
break
}
}

if hasAthena {
if len(additionalArtifacts) > 1 {
return fmt.Errorf(
"When %s exists within additional_artifacts, no other artifact type can be declared",
costandusagereportservice.AdditionalArtifactAthena,
)
}

if len(*prefix) == 0 {
return fmt.Errorf(
"When %s exists within additional_artifacts, prefix cannot be empty",
costandusagereportservice.AdditionalArtifactAthena,
)
}

if *reportVersioning != costandusagereportservice.ReportVersioningOverwriteReport {
return fmt.Errorf(
"When %s exists within additional_artifacts, report_versioning must be %s",
costandusagereportservice.AdditionalArtifactAthena,
costandusagereportservice.ReportVersioningOverwriteReport,
)
}
}

if *format == costandusagereportservice.ReportFormatParquet {
if *compression != costandusagereportservice.CompressionFormatParquet {
return fmt.Errorf(
"When format is %s, compression must also be %s",
costandusagereportservice.ReportFormatParquet,
costandusagereportservice.CompressionFormatParquet,
)
}
} else {
if *compression == costandusagereportservice.CompressionFormatParquet {
return fmt.Errorf(
"When format is %s, format must not be %s",
*format,
costandusagereportservice.CompressionFormatParquet,
)
}
}

if hasAthena && (*format != costandusagereportservice.ReportFormatParquet) {
return fmt.Errorf(
"When %s exists within additional_artifacts, both format and compression must be %s",
costandusagereportservice.AdditionalArtifactAthena,
costandusagereportservice.ReportFormatParquet,
)
}

if !hasAthena && len(additionalArtifacts) > 1 && (*format == costandusagereportservice.ReportFormatParquet) {
return fmt.Errorf(
"When additional_artifacts includes %s and/or %s, format must not be %s",
costandusagereportservice.AdditionalArtifactQuicksight,
costandusagereportservice.AdditionalArtifactRedshift,
costandusagereportservice.ReportFormatParquet,
)
}
// end checks
Copy link
Contributor

@ewbankkit ewbankkit Aug 28, 2020

Choose a reason for hiding this comment

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

Minor suggestion to move this logic into a separate function that can then be unit tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ewbankkit makes sense

@robbruce robbruce requested review from ewbankkit and removed request for a team August 28, 2020 17:31
@robbruce
Copy link
Contributor Author

@ewbankkit just as well you mentioned that as the unit test did highlight a bug

if !hasAthena && len(additionalArtifacts) > 1 && (*format == costandusagereportservice.ReportFormatParquet) {

should have been!

if !hasAthena && len(additionalArtifacts) > 0 && (*format == costandusagereportservice.ReportFormatParquet) {

@maryelizbeth maryelizbeth added enhancement Requests to existing resources that expand the functionality or scope. and removed needs-triage Waiting for first response or review from a maintainer. labels Aug 28, 2020
Copy link
Contributor

@ewbankkit ewbankkit left a comment

Choose a reason for hiding this comment

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

LGTM.

$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAwsCurReportDefinition_basic' 
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAwsCurReportDefinition_basic -timeout 120m
=== RUN   TestAccAwsCurReportDefinition_basic
=== PAUSE TestAccAwsCurReportDefinition_basic
=== CONT  TestAccAwsCurReportDefinition_basic
--- PASS: TestAccAwsCurReportDefinition_basic (14.42s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	14.466s

@bflad bflad self-assigned this Sep 2, 2020
@bflad bflad added this to the v3.5.0 milestone Sep 2, 2020
Copy link
Contributor

@bflad bflad 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 for these updates, @robbruce 🚀

Output from acceptance testing:

--- PASS: TestAccAwsCurReportDefinition_overwrite (37.09s)
--- PASS: TestAccAwsCurReportDefinition_athena (37.35s)
--- PASS: TestAccAwsCurReportDefinition_refresh (38.27s)
--- PASS: TestAccAwsCurReportDefinition_textOrCsv (38.99s)
--- PASS: TestAccAwsCurReportDefinition_basic (39.75s)
--- PASS: TestAccAwsCurReportDefinition_parquet (40.34s)

--- PASS: TestAccDataSourceAwsCurReportDefinition_basic (40.55s)
--- PASS: TestAccDataSourceAwsCurReportDefinition_additional (40.62s)

additionalArtifactsList = append(additionalArtifactsList, *additionalArtifacts[i])
}

err := checkAwsCurReportDefinitionPropertyCombination(
Copy link
Contributor

Choose a reason for hiding this comment

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

Please note that if this function causes more bug reports than its solving, the maintainers prefer to not include logic like this and will remove its maintenance burden. API error messaging concerns should be handled by the upstream service team via AWS Support cases.

@@ -107,15 +317,13 @@ func testAccPreCheckAWSCur(t *testing.T) {
func testAccAwsCurReportDefinitionConfig_basic(reportName string, bucketName string) string {
return fmt.Sprintf(`
provider "aws" {
region = "us-east-1"
region = "us-east-1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Tabs versus spaces here and below

@bflad bflad merged commit 6b40959 into hashicorp:master Sep 2, 2020
bflad added a commit that referenced this pull request Sep 2, 2020
@ghost
Copy link

ghost commented Sep 3, 2020

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

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

@ghost
Copy link

ghost commented Oct 3, 2020

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!

@ghost ghost locked as resolved and limited conversation to collaborators Oct 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Introduces or discusses updates to documentation. enhancement Requests to existing resources that expand the functionality or scope. size/XL Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants