New Resource: appsync_graphql_api #2494

atsushi-ishibashi · 2017-11-30T15:54:54Z

docs

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

radeksimko

Hi @atsushi-ishibashi
thanks for the PR, this is looking pretty good, very close to mergeable state!

I left you some comments to address.

radeksimko · 2018-02-08T12:33:29Z

aws/resource_aws_appsync_graphql_api.go

+	resp, err := conn.GetGraphqlApi(input)
+	if err != nil {
+		if isAWSErr(err, appsync.ErrCodeNotFoundException, "") {
+			d.SetId("")


Do you mind adding our usual WARN log message here?

radeksimko · 2018-02-08T12:42:33Z

aws/resource_aws_appsync_graphql_api.go

+				},
+				StateFunc: func(v interface{}) string {
+					return strings.ToUpper(v.(string))
+				},


Can't we avoid this just by tightening the validation above (i.e. by not accepting lowercase)?

radeksimko · 2018-02-08T12:46:08Z

aws/resource_aws_appsync_graphql_api.go

+						}
+					}
+					errors = append(errors, fmt.Errorf("expected %s to be one of %v, got %s", k, validTypes, value))
+					return


Would you mind using constants available in the SDK?
Also we have a built-in validation helper for this:

ValidateFunc: validation.StringInSlice([]string{ appsync.AuthenticationTypeApiKey, appsync.AuthenticationTypeAwsIam, appsync.AuthenticationTypeAmazonCognitoUserPools, }, false),

😉

radeksimko · 2018-02-08T12:47:22Z

aws/resource_aws_appsync_graphql_api.go

+							},
+							StateFunc: func(v interface{}) string {
+								return strings.ToUpper(v.(string))
+							},


Same question as above ^

radeksimko · 2018-02-08T12:47:40Z

aws/resource_aws_appsync_graphql_api.go

+								}
+								errors = append(errors, fmt.Errorf("expected %s to be one of %v, got %s", k, validTypes, value))
+								return
+							},


Same question as above ^

radeksimko · 2018-02-08T12:51:28Z

aws/resource_aws_appsync_graphql_api.go

+		return err
+	}
+
+	d.SetId("")


radeksimko · 2018-02-08T12:53:46Z

aws/resource_aws_appsync_graphql_api.go

+		if isAWSErr(err, appsync.ErrCodeNotFoundException, "") {
+			d.SetId("")
+			return nil
+		}


I can't think of a reason why the user would want to silently fail updating API when it doesn't exist. 🤔 Is there any?

The most common cases (refresh, deletion) are covered correctly, but I think we should just error out on update.

radeksimko · 2018-02-08T12:54:59Z

aws/resource_aws_appsync_graphql_api.go

+	input := &appsync.CreateGraphqlApiInput{
+		AuthenticationType: aws.String(d.Get("authentication_type").(string)),
+		Name:               aws.String(d.Get("name").(string)),
+		UserPoolConfig:     expandAppsyncGraphqlApiUserPoolConfig(d.Get("user_pool_config").([]interface{})),


I am admittedly surprised this isn't causing a crash in the attached test where user_pool_config is not present. It's no big deal as it's not crashing, but it may be safer to always wrap optional fields in d.GetOk().

radeksimko · 2018-02-08T12:57:02Z

website/docs/r/appsync_graphql_api.html.markdown

+---
+layout: "aws"
+page_title: "AWS: aws_appsync_graphql_api"
+sidebar_current: "docs-aws-resource-appsync-praphql-api"


Typo 👓 praphql -> graphql

radeksimko · 2018-02-08T12:58:36Z

aws/resource_aws_appsync_graphql_api_test.go

+  authentication_type = "AMAZON_COGNITO_USER_POOLS"
+  name = "tf_appsync_%s"
+  user_pool_config {
+    aws_region = "us-west-2"


Although our tests commonly run in us-west-2 I think we should keep the test region-agnostic. Do you mind adding aws_region data source here?

https://www.terraform.io/docs/providers/aws/d/region.html

atsushi-ishibashi · 2018-02-10T07:33:35Z

@radeksimko Thank you for your review. OK👍

TF_ACC=1 go test ./aws -v -run=TestAccAwsAppsyncGraphqlApi_* -timeout 120m
=== RUN   TestAccAwsAppsyncGraphqlApi_basic
--- PASS: TestAccAwsAppsyncGraphqlApi_basic (32.12s)
=== RUN   TestAccAwsAppsyncGraphqlApi_iam
--- PASS: TestAccAwsAppsyncGraphqlApi_iam (27.64s)
=== RUN   TestAccAwsAppsyncGraphqlApi_cognito
--- PASS: TestAccAwsAppsyncGraphqlApi_cognito (44.92s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws

radeksimko

LGTM, Thanks, I just removed the not-found check from Update() as mentioned. I hope you don't mind.

atsushi-ishibashi · 2018-02-12T13:42:36Z

@radeksimko Ok👍I misunderstood what you said in the above review.
Thank you!

ahl · 2018-02-25T14:04:03Z

Is the plan to accrete pieces for appsync gradually or is there a consistent plan? @atsushi-ishibashi

atsushi-ishibashi · 2018-02-25T14:23:00Z

I have no plan to implement anymore. In my opinion, schema, resolver and type are outside of terraform's role and apikey will expire in a few days so it also doesn't suit.
But they are important for those who want to manage them by terraform👍

bflad · 2018-02-27T12:40:45Z

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

ghost · 2020-04-07T17:55:28Z

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!

radeksimko added the new-resource Introduces a new resource. label Nov 30, 2017

atsushi-ishibashi mentioned this pull request Dec 3, 2017

vendor: appsync@v1.12.38 #2518

Merged

atsushi-ishibashi added 2 commits December 23, 2017 00:46

[WIP]New Resource: appsync_graphql_api

9b379d3

testacc

910366b

atsushi-ishibashi force-pushed the appsync_graphql_api branch from 656cdd5 to 910366b Compare December 22, 2017 16:18

docs

813875c

atsushi-ishibashi changed the title ~~[WIP]New Resource: appsync_graphql_api~~ New Resource: appsync_graphql_api Dec 23, 2017

atsushi-ishibashi mentioned this pull request Dec 23, 2017

New Resource: aws_appsync_datasource #2758

Merged

2 tasks

fix typo

f8e7c19

radeksimko mentioned this pull request Jan 9, 2018

AppSync resource proposal #2705

Closed

radeksimko added the service/appsync Issues and PRs that pertain to the appsync service. label Jan 16, 2018

radeksimko reviewed Feb 8, 2018

View reviewed changes

radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Feb 8, 2018

update

210d7a8

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

Remove not-found check from Update

c8bca34

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

radeksimko removed the waiting-response Maintainers are waiting on response from community or contributor. label Feb 12, 2018

radeksimko approved these changes Feb 12, 2018

View reviewed changes

radeksimko merged commit 8f671af into hashicorp:master Feb 12, 2018

bflad added this to the v1.10.0 milestone Feb 27, 2018

ghost locked and limited conversation to collaborators Apr 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: appsync_graphql_api #2494

New Resource: appsync_graphql_api #2494

atsushi-ishibashi commented Nov 30, 2017 •

edited

Loading

radeksimko left a comment

radeksimko Feb 8, 2018

radeksimko Feb 8, 2018

radeksimko Feb 8, 2018

radeksimko Feb 8, 2018

radeksimko Feb 8, 2018

radeksimko Feb 8, 2018

radeksimko Feb 8, 2018

radeksimko Feb 8, 2018

radeksimko Feb 8, 2018

radeksimko Feb 8, 2018

atsushi-ishibashi commented Feb 10, 2018

radeksimko left a comment

atsushi-ishibashi commented Feb 12, 2018 •

edited

Loading

ahl commented Feb 25, 2018

atsushi-ishibashi commented Feb 25, 2018 •

edited

Loading

bflad commented Feb 27, 2018

ghost commented Apr 7, 2020

New Resource: appsync_graphql_api #2494

New Resource: appsync_graphql_api #2494

Conversation

atsushi-ishibashi commented Nov 30, 2017 • edited Loading

radeksimko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atsushi-ishibashi commented Feb 10, 2018

radeksimko left a comment

Choose a reason for hiding this comment

atsushi-ishibashi commented Feb 12, 2018 • edited Loading

ahl commented Feb 25, 2018

atsushi-ishibashi commented Feb 25, 2018 • edited Loading

bflad commented Feb 27, 2018

ghost commented Apr 7, 2020

atsushi-ishibashi commented Nov 30, 2017 •

edited

Loading

atsushi-ishibashi commented Feb 12, 2018 •

edited

Loading

atsushi-ishibashi commented Feb 25, 2018 •

edited

Loading