-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Add app sync schema #4840
Add app sync schema #4840
Conversation
I could really use appsync schemas and resolvers for a current project! I'd love to see this get in. |
@bflad Is there anything more I need to do with this to get it pulled in? |
Looks like the travis CI checks passed. I would LOVE to use terraform exclusively for my schema updates, rather than a hybrid where I do some parts of AppSync with TF and must do the remainder with CF. I hope this gets added soon! |
Wow! This will make my life so much better when it lands - hope it can be soon too! (@phuonglu) |
+1 from me. We really need this for our Terraform setup. 👍 |
+1 from me 👍 |
Hi folks 👋 Please note that we can only utilize 👍 upvote reactions to the original issue or pull request for reporting and prioritization. e.g. https://github.com/terraform-providers/terraform-provider-aws/issues?q=is%3Aissue+is%3Aopen+sort%3Areactions-%2B1-desc "+1"/"me too" comments unfortunately only generate noise for issue/pull request followers and do not help the maintainers with prioritization. 😅 |
👍 upvote +1 |
Yea, this would be nice, it’s pretty bobo to have to shell out to,get this done |
yes, please |
+1. Like to see this implemented. |
+1.This really needs to be added. |
@beauknowssoftware, perhaps it would be easier if you extract your changes to be a separate plugin? In this case, we won't need to wait for ages till it gets merged into master... |
aws/resource_aws_appsync_schema.go
Outdated
return err | ||
} | ||
|
||
if err := waitForSchemaToBeActive(apiId, meta); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@beauknowssoftware I've been working on implementing AppSync resolvers and came across your PR via #2705 . I'm wondering if this wait
could be implemented using StateChangeConf
provided in terraform/helper/resource. Here's an example in CloudFormation stacks: https://github.com/terraform-providers/terraform-provider-aws/blob/master/aws/resource_aws_cloudformation_stack.go#L178. I believe it can since there are discrete statuses returned from GetSchemaCreationStatus
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will agree with @perryao here -- we tend to prefer API waiting via existing SDK Waiter
functions (pretty rare), resource.Retry()
, or resource.StateChangeConf
: https://www.terraform.io/docs/extend/resources.html#statechangeconf
There are quite a few examples across the project if you search for StateChangeConf
, but I can certainly help if you need specific guidance. 😄
Does this include resolver configuration or just the schema? |
Any progress on this? AppSync has been in general availability for 10 months .... |
As a workaround, I use cloudformation to define schema and resolvers for now. Hope it helps to someone who also waits for this PR to be merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @beauknowssoftware 👋 Thanks for submitting this and sorry for the delays in getting it reviewed. 😅 I left some initial questions and feedback below. Please reach out with any questions or let us know if you do not have time to continue working on this.
aws/resource_aws_appsync_schema.go
Outdated
func resourceAwsAppsyncSchema() *schema.Resource { | ||
return &schema.Resource{ | ||
Create: resourceAwsAppsyncSchemaPut, | ||
Read: resourceAwsAppsyncSchemaNil, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to read this information via the GetIntrospectionSchema
API call? One of Terraform's design tenets is that it can detect configuration drift. If its not possible to read this information back from the API, this may not be a good candidate for management in Terraform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The introspection schema is not the same blob that is sent to StartSchemaCreation. These are two different objects. Schema must be present in order to create resolvers. Setting up the schema outside of Terraform would mean either setting up the API outside of Terraform before hand or setting up resolvers outside of Terraform afterwards. Neither of these options is ideal.
aws/provider.go
Outdated
@@ -309,6 +309,7 @@ func Provider() terraform.ResourceProvider { | |||
"aws_appsync_api_key": resourceAwsAppsyncApiKey(), | |||
"aws_appsync_datasource": resourceAwsAppsyncDatasource(), | |||
"aws_appsync_graphql_api": resourceAwsAppsyncGraphqlApi(), | |||
"aws_appsync_schema": resourceAwsAppsyncSchema(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can a AppSync Graph API have more than one schema? If not, instead of a new resource, it may make better sense as an attribute of the existing aws_appsync_graphql_api
resource (unless self-references are necessary).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is only one schema per API. I can look into adding it as an attribute instead of a separate resource. This would move my StartSchemaCreation logic into the API create/update methods.
aws/resource_aws_appsync_schema.go
Outdated
return err | ||
} | ||
|
||
if err := waitForSchemaToBeActive(apiId, meta); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will agree with @perryao here -- we tend to prefer API waiting via existing SDK Waiter
functions (pretty rare), resource.Retry()
, or resource.StateChangeConf
: https://www.terraform.io/docs/extend/resources.html#statechangeconf
There are quite a few examples across the project if you search for StateChangeConf
, but I can certainly help if you need specific guidance. 😄
aws/resource_aws_appsync_schema.go
Outdated
Create: resourceAwsAppsyncSchemaPut, | ||
Read: resourceAwsAppsyncSchemaNil, | ||
Update: resourceAwsAppsyncSchemaPut, | ||
Delete: resourceAwsAppsyncSchemaNil, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not having a deletion method may be another sign that this may not make sense as its own resource, but instead an attribute of an existing "parent" resource (aws_appsync_graphql_api
). 👍 Otherwise, we should use schema.Noop
instead of our own function here and add a warning to the documentation that removing this resource does not actually delete the schema.
aws/resource_aws_appsync_schema.go
Outdated
Delete: resourceAwsAppsyncSchemaNil, | ||
|
||
Schema: map[string]*schema.Schema{ | ||
"api_id": &schema.Schema{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This attribute should be ForceNew: true
since this field is not update-able.
aws/resource_aws_appsync_schema.go
Outdated
if d.HasChange("definition") { | ||
input := &appsync.StartSchemaCreationInput{ | ||
ApiId: aws.String(apiId), | ||
Definition: ([]byte)(d.Get("definition").(string)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this attribute is []byte
, do we need to worry about actual binary data? Terraform's state currently requires UTF-8 compatibility otherwise it could cause corruption.
has @beauknowssoftware gone MIA? Is this still in-progress? Just curious, thanks for working on this. |
Sorry for the late response to this thread. I haven't forgotten about this PR, but I also haven't had time to get back to work on it. Hoping to have more spare time over the holidays |
+1 for this please. |
hey @beauknowssoftware any chance of an update? cheers for your work so far ;) |
7bfbaf5
to
433afcf
Compare
c9f7276
to
492ada1
Compare
492ada1
to
078d9a6
Compare
078d9a6
to
3a4927b
Compare
3a4927b
to
666077b
Compare
@bflad |
a85a7c2
to
666077b
Compare
TF_ACC=1 go test ./... -v -parallel 20 -run="AccAWSAppsyncGraphqlApi_basic" -timeout 120m |
This reverts commit a85a7c2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much @beauknowssoftware for the updates. 🚀 I left some minor feedback items below -- some of which are being addressed on merge in a followup commit and others in the followup #6451 pull request which builds on this work here. Thanks again.
Output from acceptance testing:
--- PASS: TestAccAWSAppsyncGraphqlApi_AuthenticationType_AWSIAM (11.52s)
--- PASS: TestAccAWSAppsyncGraphqlApi_AuthenticationType_APIKey (11.59s)
--- PASS: TestAccAWSAppsyncGraphqlApi_basic (12.23s)
--- PASS: TestAccAWSAppsyncGraphqlApi_AuthenticationType_OpenIDConnect (12.81s)
--- PASS: TestAccAWSAppsyncGraphqlApi_AuthenticationType_AmazonCognitoUserPools (13.28s)
--- PASS: TestAccAWSAppsyncGraphqlApi_LogConfig (13.38s)
--- PASS: TestAccAWSAppsyncGraphqlApi_OpenIDConnectConfig_IatTTL (15.68s)
--- PASS: TestAccAWSAppsyncGraphqlApi_OpenIDConnectConfig_Issuer (17.19s)
--- PASS: TestAccAWSAppsyncGraphqlApi_OpenIDConnectConfig_ClientID (17.31s)
--- PASS: TestAccAWSAppsyncGraphqlApi_UserPoolConfig_DefaultAction (19.40s)
--- PASS: TestAccAWSAppsyncGraphqlApi_UserPoolConfig_AwsRegion (19.53s)
--- PASS: TestAccAWSAppsyncGraphqlApi_schema (22.54s)
--- PASS: TestAccAWSAppsyncGraphqlApi_LogConfig_FieldLogLevel (22.98s)
--- PASS: TestAccAWSAppsyncGraphqlApi_Name (23.53s)
--- PASS: TestAccAWSAppsyncGraphqlApi_OpenIDConnectConfig_AuthTTL (25.50s)
--- PASS: TestAccAWSAppsyncGraphqlApi_AuthenticationType (27.44s)
@@ -159,6 +165,11 @@ func resourceAwsAppsyncGraphqlApiCreate(d *schema.ResourceData, meta interface{} | |||
return err | |||
} | |||
|
|||
err = resourceAwsAppsyncSchemaPut(*resp.GraphqlApi.ApiId, d, meta) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This handling should be removed below the d.SetId()
call so Terraform does not lose track of the AppSync GraphQL API resource if this errors.
@@ -159,6 +165,11 @@ func resourceAwsAppsyncGraphqlApiCreate(d *schema.ResourceData, meta interface{} | |||
return err | |||
} | |||
|
|||
err = resourceAwsAppsyncSchemaPut(*resp.GraphqlApi.ApiId, d, meta) | |||
if err != nil { | |||
return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should return error context here for operators and code maintainers:
return err | |
return fmt.Errorf("error creating AppSync GraphQL API (%s) Schema: %s", d.Id(), err) |
@@ -230,6 +241,11 @@ func resourceAwsAppsyncGraphqlApiUpdate(d *schema.ResourceData, meta interface{} | |||
return err | |||
} | |||
|
|||
err = resourceAwsAppsyncSchemaPut(d.Id(), d, meta) | |||
if err != nil { | |||
return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should return error context here for operators and code maintainers:
return err | |
return fmt.Errorf("error updating AppSync GraphQL API (%s) Schema: %s", d.Id(), err) |
} | ||
|
||
activeSchemaConfig := &resource.StateChangeConf{ | ||
Pending: []string{"PROCESSING"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: We should use the AWS Go SDK provided constants here and below, e.g.
Pending: []string{"PROCESSING"}, | |
Pending: []string{appsync.SchemaStatusProcessing}, |
PreCheck: func() { testAccPreCheck(t) }, | ||
Providers: testAccProviders, | ||
CheckDestroy: testAccCheckAwsAppsyncGraphqlApiDestroy, | ||
Steps: []resource.TestStep{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the new attribute is not marked ForceNew: true
an additional TestStep
should be included to ensure updates occur correctly.
@@ -104,6 +120,7 @@ The following arguments are supported: | |||
* `log_config` - (Optional) Nested argument containing logging configuration. Defined below. | |||
* `openid_connect_config` - (Optional) Nested argument containing OpenID Connect configuration. Defined below. | |||
* `user_pool_config` - (Optional) The Amazon Cognito User Pool configuration. Defined below. | |||
* `schema` - (Required) The schema definition, in GraphQL schema language format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* `schema` - (Required) The schema definition, in GraphQL schema language format | |
* `schema` - (Optional) The schema definition, in GraphQL schema language format. Terraform cannot perform drift detection of this configuration. |
Output from acceptance testing: ``` --- PASS: TestAccAWSAppsyncGraphqlApi_AuthenticationType_AWSIAM (11.52s) --- PASS: TestAccAWSAppsyncGraphqlApi_AuthenticationType_APIKey (11.59s) --- PASS: TestAccAWSAppsyncGraphqlApi_basic (12.23s) --- PASS: TestAccAWSAppsyncGraphqlApi_AuthenticationType_OpenIDConnect (12.81s) --- PASS: TestAccAWSAppsyncGraphqlApi_AuthenticationType_AmazonCognitoUserPools (13.28s) --- PASS: TestAccAWSAppsyncGraphqlApi_LogConfig (13.38s) --- PASS: TestAccAWSAppsyncGraphqlApi_OpenIDConnectConfig_IatTTL (15.68s) --- PASS: TestAccAWSAppsyncGraphqlApi_OpenIDConnectConfig_Issuer (17.19s) --- PASS: TestAccAWSAppsyncGraphqlApi_OpenIDConnectConfig_ClientID (17.31s) --- PASS: TestAccAWSAppsyncGraphqlApi_UserPoolConfig_DefaultAction (19.40s) --- PASS: TestAccAWSAppsyncGraphqlApi_UserPoolConfig_AwsRegion (19.53s) --- PASS: TestAccAWSAppsyncGraphqlApi_schema (22.54s) --- PASS: TestAccAWSAppsyncGraphqlApi_LogConfig_FieldLogLevel (22.98s) --- PASS: TestAccAWSAppsyncGraphqlApi_Name (23.53s) --- PASS: TestAccAWSAppsyncGraphqlApi_OpenIDConnectConfig_AuthTTL (25.50s) --- PASS: TestAccAWSAppsyncGraphqlApi_AuthenticationType (27.44s) ```
This has been released in version 2.4.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. |
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! |
Changes proposed in this pull request:
Output from acceptance testing: