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

New Resource: aws_appsync_datasource #2758

Merged

radeksimko merged 7 commits into hashicorp:master from atsushi-ishibashi:appsync_datasource

Mar 28, 2018

Contributor

atsushi-ishibashi commented Dec 23, 2017 •

edited

Loading

Required: #2494

test
docs

TF_ACC=1 go test ./aws -v -run=TestAccAwsAppsyncDatasource_ -timeout 120m
=== RUN   TestAccAwsAppsyncDatasource_ddb
--- PASS: TestAccAwsAppsyncDatasource_ddb (59.92s)
=== RUN   TestAccAwsAppsyncDatasource_es
--- PASS: TestAccAwsAppsyncDatasource_es (996.38s)
=== RUN   TestAccAwsAppsyncDatasource_lambda
--- PASS: TestAccAwsAppsyncDatasource_lambda (55.18s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws

Contributor Author

atsushi-ishibashi commented Dec 23, 2017

TF_ACC=1 go test ./aws -v -run=TestAccAwsAppsyncDatasource_update -timeout 120m
=== RUN   TestAccAwsAppsyncDatasource_update
--- PASS: TestAccAwsAppsyncDatasource_update (103.41s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	103.443s

radeksimko added the new-data-source label

atsushi-ishibashi changed the title ~~[WIP]New Resource: Appsync datasource~~ New Resource: Appsync datasource

Contributor Author

atsushi-ishibashi commented Dec 25, 2017

@radeksimko This is new-resource, not new-data-source. The name of the object is AppSyncDataSource so it might confused you 😅

radeksimko added new-resource and removed new-data-source labels

radeksimko mentioned this pull request

AppSync resource proposal #2705

Closed

radeksimko added the service/appsync label

radeksimko changed the title ~~New Resource: Appsync datasource~~ New Resource: aws_appsync_datasource

Member

radeksimko commented Feb 8, 2018

Thanks for this PR too.

I will let you finish #2494 first though before diving into this one as it's easier to deal with smaller diff (after rebasing).

radeksimko added the waiting-response label

Member

radeksimko commented Feb 12, 2018

@atsushi-ishibashi Do you mind rebasing this PR and resolving conflicts?

atsushi-ishibashi force-pushed the appsync_datasource branch from c704c1f to c066f0d Compare

February 15, 2018 14:31

ghost added the size/XL label

radeksimko removed the waiting-response label

radeksimko reviewed

View reviewed changes

Member

radeksimko left a comment

Hi @atsushi-ishibashi
sorry for a small delay in processing this PR.

It looks overall pretty good, I just left you some comments to address about naming conventions etc.

Let me know if you have any questions, as always! 🙂

aws/resource_aws_appsync_datasource.go Outdated

    
              				Required: true,

              				ValidateFunc: func(v interface{}, k string) (ws []string, errors []error) {

              					value := strings.ToUpper(v.(string))

              					validTypes := []string{"AWS_LAMBDA", "AMAZON_DYNAMODB", "AMAZON_ELASTICSEARCH"}

Member

radeksimko Mar 26, 2018

Do you mind turning these into constants and using a helper here? i.e.

ValidateFunc: validation.StringInSlice([]string{
    appsync.DataSourceTypeAwsLambda,
    appsync.DataSourceTypeAmazonDynamodb,
    appsync.DataSourceTypeAmazonElasticsearch,
    appsync.DataSourceTypeNone,
}, true),

aws/resource_aws_appsync_datasource.go Outdated

    
              				},

              				ConflictsWith: []string{"dynamodb_config", "elasticsearch_config"},

              			},

              			"service_role": {

Member

radeksimko Mar 26, 2018

I think that we should keep this name as specific as it is in the API, i.e. service_role_arn, so it's obvious that we expect ARN here, not ID or name or anything else.

aws/resource_aws_appsync_datasource.go Outdated

    
              							Type:     schema.TypeString,

              							Required: true,

              						},

              						"table": {

Member

radeksimko Mar 26, 2018

I think that we should keep this name as specific as it is in the API, i.e. table_name, so it's obvious that we expect name of the table here, not ID/ARN or anything else.

aws/resource_aws_appsync_datasource.go Outdated

    
              				MaxItems: 1,

              				Elem: &schema.Resource{

              					Schema: map[string]*schema.Schema{

              						"arn": {

Member

radeksimko Mar 26, 2018

I think that we should make this name a bit more specific, e.g. function_arn, so it's obvious that we expect ARN of the Lambda function - just in case Amazon decides to introduce more ARN-based arguments in the future.

aws/resource_aws_appsync_datasource.go Outdated

    
              	resp, err := conn.GetDataSource(input)

              	if err != nil {

              		if isAWSErr(err, appsync.ErrCodeNotFoundException, "") {

              			d.SetId("")

Member

radeksimko Mar 26, 2018

Do you mind adding a WARN log message here per convention?

aws/resource_aws_appsync_datasource.go Outdated

    
              	_, err := conn.DeleteDataSource(input)

              	if err != nil {

              		if isAWSErr(err, appsync.ErrCodeNotFoundException, "") {

              			d.SetId("")

Member

radeksimko Mar 26, 2018

The SetId("") is redundant here as we empty ID (and wipe the resource) in the upstream library inside Terraform's core.

aws/resource_aws_appsync_datasource.go Outdated

    
              		return err

              	}

              	d.SetId("")

Member

radeksimko Mar 26, 2018

The SetId("") is redundant here as we empty ID (and wipe the resource) in the upstream library inside Terraform's core.

aws/resource_aws_appsync_datasource_test.go

Member

radeksimko Mar 26, 2018

Thanks for the thorough test coverage here! 👍

aws/resource_aws_appsync_datasource_test.go Outdated

    
                name = "tf_appsync_%s"

                type = "AMAZON_DYNAMODB"

                dynamodb_config {

                  region = "us-west-2"

Member

radeksimko Mar 26, 2018

Do you mind swapping this for a data source, so the test is usable in any region?
https://www.terraform.io/docs/providers/aws/d/region.html

aws/resource_aws_appsync_datasource_test.go Outdated

    
                name = "tf_appsync_%s"

                type = "AMAZON_ELASTICSEARCH"

                elasticsearch_config {

                  region = "us-west-2"

Member

radeksimko Mar 26, 2018

Do you mind swapping this for a data source, so the test is usable in any region?
https://www.terraform.io/docs/providers/aws/d/region.html

radeksimko added the waiting-response label

atsushi-ishibashi added 6 commits

March 27, 2018 00:46

wip

7a1bba3


          add test


          Add docs

ff4684d


          modify

26591b2


          reflect reviews

25bbbfd


          add resource provider

fcd7f25

atsushi-ishibashi force-pushed the appsync_datasource branch from 16684aa to fcd7f25 Compare

March 26, 2018 17:05

ghost added the size/XL label

Contributor Author

atsushi-ishibashi commented Mar 26, 2018

I have no idea why below error occured. 🤔

TF_ACC=1 go test ./aws -v -run=TestAccAwsAppsyncDatasource_* -timeout 120m
=== RUN   TestAccAwsAppsyncDatasource_ddb
--- PASS: TestAccAwsAppsyncDatasource_ddb (86.84s)
=== RUN   TestAccAwsAppsyncDatasource_es
--- FAIL: TestAccAwsAppsyncDatasource_es (322.16s)
	testing.go:518: Step 0 error: Error applying: 1 error(s) occurred:
		
		* aws_appsync_datasource.test: 1 error(s) occurred:
		
		* aws_appsync_datasource.test: BadRequestException: Invalid endpoint configuration.
			status code: 400, request id: 5120e276-310f-11e8-b4d3-fdc8c0696d6a
=== RUN   TestAccAwsAppsyncDatasource_lambda
--- PASS: TestAccAwsAppsyncDatasource_lambda (56.85s)
=== RUN   TestAccAwsAppsyncDatasource_update
--- PASS: TestAccAwsAppsyncDatasource_update (109.15s)
FAIL
FAIL	github.com/terraform-providers/terraform-provider-aws/aws	575.030s
make: *** [testacc] Error 1


          Add missing protocol to endpoint

55c47c8

radeksimko approved these changes

View reviewed changes

Member

radeksimko left a comment

@atsushi-ishibashi the test failure was related to missing protocol in the endpoint, I just patched the test so we can pull this in as the PR is otherwise in a great shape. Thanks.

radeksimko removed new-resource waiting-response labels

radeksimko added the new-resource label

radeksimko merged commit 13a8273 into hashicorp:master

bflad added this to the v1.13.0 milestone

Contributor

bflad commented Mar 29, 2018

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

lukehoban mentioned this pull request

[appsync] support for serverless aurora rds data source missing pulumi/pulumi-aws#461

Closed

ghost commented Apr 7, 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 locked and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

new-resource service/appsync size/XL