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

Issue #5774 - Fix the update issues in aws_appsync_datasource #5814

Merged
merged 4 commits into from
Oct 5, 2018
Merged

Issue #5774 - Fix the update issues in aws_appsync_datasource #5814

merged 4 commits into from
Oct 5, 2018

Conversation

saravanan30erd
Copy link
Contributor

Fixes #5774

Changes proposed in this pull request:

  • Issue 1
=== RUN   TestAccAwsAppsyncDatasource_update
--- FAIL: TestAccAwsAppsyncDatasource_update (294.67s)
    testing.go:527: Step 1 error: Error applying: 1 error occurred:
        	* aws_appsync_datasource.test: 1 error occurred:
        	* aws_appsync_datasource.test: InvalidParameter: 1 validation error(s) found.
        - missing required field, UpdateDataSourceInput.Type.

Type is required field, added in construct.

  • Issue 2
=== RUN   TestAccAwsAppsyncDatasource_update
--- FAIL: TestAccAwsAppsyncDatasource_update (279.02s)
    testing.go:527: Step 1 error: Error applying: 1 error occurred:
        	* aws_appsync_datasource.test: 1 error occurred:
        	* aws_appsync_datasource.test: BadRequestException: Service role arn not specified
        	status code: 400, request id: ccf048dd-b343-11e8-807f-1d73b8da92e0

If the datasource type is either dynamodb, elasticsearch or lambda, it requires service_role_arn.

Output from acceptance testing:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAwsAppsyncDatasource_update'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAwsAppsyncDatasource_update -timeout 120m
=== RUN   TestAccAwsAppsyncDatasource_updateType
--- PASS: TestAccAwsAppsyncDatasource_updateType (119.73s)
=== RUN   TestAccAwsAppsyncDatasource_update
--- PASS: TestAccAwsAppsyncDatasource_update (240.17s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	359.938s
...

@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Sep 8, 2018
@bflad bflad added bug Addresses a defect in current functionality. service/appsync Issues and PRs that pertain to the appsync service. labels Sep 11, 2018
bflad
bflad reviewed Sep 11, 2018
View reviewed changes
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.

Initial feedback below -- please let us know if you have any questions or do not have time to implement the changes. Thanks!

aws/resource_aws_appsync_datasource_test.go Outdated
@@ -188,6 +213,7 @@ resource "aws_appsync_datasource" "test" {
api_id = "${aws_appsync_graphql_api.test.id}"
name = "tf_appsync_%s"
type = "AMAZON_DYNAMODB"
description = "appsync datasource version1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: Generally its cleaner to parameterize the test configuration values when changing a single one instead of creating a mostly duplicated new test configuration, e.g.

func testAccAppsyncDatasourceConfig_ddb(rName, description string) string {
// ...
description = %q
// ...
`, rName, rName, rName, rName, rName, rName, description)
}

aws/resource_aws_appsync_datasource.go Outdated
}

if d.HasChange("description") {
input.Description = aws.String(d.Get("description").(string))
}
if d.HasChange("service_role_arn") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Given this type of change, I'm worried that all parameters must be provided during the update call. We should probably add an acceptance test that has something optional like description set, then attempts to update something else like dynamodb_config, then verifies that description is still the original value as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bflad Reverted the changes which I did for service_role_arn attr to provide the proper solution.
As you said, looks like all parameters should be provided during the update call and covered this issue with acceptance test TestAccAwsAppsyncDatasource_update, working on fix.

Copy link
Contributor Author

@saravanan30erd saravanan30erd Sep 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@bflad Could you guide me the way how to handle this?
Since all parameters need to be provided in update call, I am thinking to use Get funcs.

@bflad bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Sep 11, 2018
@ghost ghost added size/M Managed by automation to categorize the size of a PR. service/appsync Issues and PRs that pertain to the appsync service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. and removed size/L Managed by automation to categorize the size of a PR. labels Sep 18, 2018
@ghost ghost added size/L Managed by automation to categorize the size of a PR. service/appsync Issues and PRs that pertain to the appsync service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. and removed size/M Managed by automation to categorize the size of a PR. labels Sep 21, 2018
@saravanan30erd
Copy link
Contributor Author

@bflad used Get funcs for update.

$ make testacc TEST=./aws TESTARGS='-run=TestAccAwsAppsyncDatasource_update'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAwsAppsyncDatasource_update -timeout 120m
=== RUN TestAccAwsAppsyncDatasource_update
--- PASS: TestAccAwsAppsyncDatasource_update (119.33s)
=== RUN TestAccAwsAppsyncDatasource_updateDescription
--- PASS: TestAccAwsAppsyncDatasource_updateDescription (105.89s)
PASS
ok github.com/terraform-providers/terraform-provider-aws/aws 225.268s

@bflad bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label Oct 5, 2018
bflad
bflad approved these changes Oct 5, 2018
View reviewed changes
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.

Thanks, @saravanan30erd! 🚀

5 tests passed (all tests)
--- PASS: TestAccAwsAppsyncDatasource_ddb (22.58s)
--- PASS: TestAccAwsAppsyncDatasource_lambda (23.28s)
--- PASS: TestAccAwsAppsyncDatasource_updateDescription (27.18s)
--- PASS: TestAccAwsAppsyncDatasource_update (40.85s)
--- PASS: TestAccAwsAppsyncDatasource_es (772.27s)

aws/resource_aws_appsync_datasource_test.go
@@ -46,13 +46,14 @@ func TestAccAwsAppsyncDatasource_es(t *testing.T) {
}

func TestAccAwsAppsyncDatasource_lambda(t *testing.T) {
Desc := "appsync datasource"
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, starting variable names with a capital letter is special in Go (makes them exportable). While it shouldn't matter in this case, its generally best practice to lowerCamelCase unless otherwise necessary.

@bflad bflad added this to the v1.40.0 milestone Oct 5, 2018
@bflad bflad merged commit de0f8f8 into hashicorp:master Oct 5, 2018
bflad added a commit that referenced this pull request Oct 5, 2018
@bflad
Update CHANGELOG for #5814
2bdc70d
@bflad
Copy link
Contributor

bflad commented Oct 10, 2018

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

@ghost
Copy link

ghost commented Apr 2, 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 and limited conversation to collaborators Apr 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bflad bflad bflad approved these changes

Assignees
No one assigned
Labels
bug Addresses a defect in current functionality. service/appsync Issues and PRs that pertain to the appsync service. size/L 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
Milestone
v1.40.0
Development

Successfully merging this pull request may close these issues.

Any attempt to update aws_appsync_datasource.dynamodb_datasource results in the following error
2 participants
@saravanan30erd @bflad