-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
New Resource: aws_appsync_api_key #3827
Conversation
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 @lucidprogrammer 👋 Thanks for this contribution! I have left some initial review feedback. Please let me know if you have any questions or don't have time to implement these items. Thanks!
aws/resource_aws_appsync_api_key.go
Outdated
Type: schema.TypeString, | ||
Required: true, | ||
}, | ||
"valid_till_date": { |
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.
A few notes here:
- We should match the AWS API and create a single attribute
expires
rather than provide multiple conflicting attributes for the same information (dropvalid_till_date
,validity_period_days
, andexpiry_date
) - It should accept a RFC3339 timestamp for now and can be validated via
ValidateFunc: validation.ValidateRFC3339TimeString
aws/resource_aws_appsync_api_key.go
Outdated
func resourceAwsAppsyncApiKeyRead(d *schema.ResourceData, meta interface{}) error { | ||
conn := meta.(*AWSClient).appsyncconn | ||
|
||
input := &appsync.ListApiKeysInput{ |
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 need to account for NextToken
when the list is larger than the returned results or we could potentially miss fetching the API key.
aws/resource_aws_appsync_api_key.go
Outdated
if err != nil { | ||
return err | ||
} | ||
var key appsync.ApiKey |
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 code is not accounting for if the key is not found in the ListApiKeys
output(s). I would recommend changing this to var key *appsync.ApiKey
and performing a nil check before dereferencing the values (otherwise it will cause a Terraform panic):
// After exhausting all ListApiKeys searching
if key == nil {
log.Printf("[WARN] AppSync API Key %q not found, removing from state", d.Id())
d.SetId("")
return nil
}
d.Set("key", key.Id)
// ...
aws/resource_aws_appsync_api_key.go
Outdated
conn := meta.(*AWSClient).appsyncconn | ||
|
||
params := &appsync.UpdateApiKeyInput{ | ||
ApiId: aws.String(d.Get("appsync_api_id").(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.
Given the API key seems a subset of the API ID, we should probably add this ID to the Terraform resource ID.
In the Create function (if :
is a safe delimiter):
d.SetId(fmt.Sprintf("%s:%s"), d.Get("appsync_api_id").(string), *resp.ApiKey.Id)
We can then parse it with a helper function, e.g.
func decodeAppSyncApiKeyId(id string) (string, string, error) {
parts := strings.Split(id, ":")
if len(parts) != 2 {
return "", "", fmt.Errorf("Unexpected format of ID (%q), expected API-ID:API-KEY-ID", id)
}
return parts[0], parts[1], nil
}
And anywhere necessary to call that:
apiID, apiKeyID, err := decodeAppSyncApiKeyId(d.Id())
if err != nil {
return err
}
This will allow us to support importing easily with:
Importer: &schema.ResourceImporter{
State: schema.ImportStatePassthrough,
},
aws/resource_aws_appsync_api_key.go
Outdated
Optional: true, | ||
Default: "Managed by Terraform", | ||
}, | ||
"appsync_api_id": { |
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 don't think we need to prepend appsync_
to this attribute name. 👍
@bflad I will update docs tomorrow related to the resource changes later tonight |
@bflad anything else to be done on this, let me know your thoughts. tks |
@bflad i know it could be hectic with lot going on at your end. doing this for a customer, and pressure is mounting, where other resources for appsync also to be created.(like schema creation, types etc). any feedback to merge this will be much appreciated. tks |
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 think this is almost there, just two more things and should be good to go. 👍
if er != nil { | ||
return er | ||
} | ||
listKeys = func(input *appsync.ListApiKeysInput) (*appsync.ApiKey, error) { |
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 should be made into its own top level function, so it can be used by testAccCheckAwsAppsyncApiKeyDestroy
and testAccCheckAwsAppsyncApiKeyExists
so they properly handle NextToken
var key appsync.ApiKey | ||
for _, v := range resp.ApiKeys { | ||
if *v.Id == *aws.String(Id) { | ||
key = *v |
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 would recommend splitting this logic into two functions:
func testAccCheckAwsAppsyncApiKeyExists(resourceName string, apiKey *appsync.ApiKey)
which calls *apiKey = *v
right here or otherwise returns an error that the key could not be found.
Then separately:
func testAccCheckAwsAppsyncApiKeyTillDate(apiKey *appsync.ApiKey, date time.Time)
which handles the date/time logic below.
Finished the implementation in a commit after the others. Passes acceptance testing:
|
This has been released in version 1.27.0 of the 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! |
Supporting the creation of appsync api key. Either a specific date or number of days.
resource "aws_appsync_graphql_api" "example" {
authentication_type = "API_KEY"
name = "example"
}
resource "aws_appsync_api_key" "ex1" {
appsync_api_id = "${aws_appsync_graphql_api.example.id}"
validity_period_days = 364
}
resource "aws_appsync_api_key" "ex2" {
appsync_api_id = "${aws_appsync_graphql_api.example.id}"
valid_till_date = "30/11/2018"
}