-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
r/kinesis_analytics_application: new resource #5456
Conversation
CurrentApplicationVersionId: aws.Int64(int64(version)), | ||
CloudWatchLoggingOption: cloudwatchLoggingOption, | ||
} | ||
conn.AddApplicationCloudWatchLoggingOption(addOpts) |
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 really sure the best way to handle this but adding new cloudwatch logging options cannot be part of the UpdateApplication
call hence a separate call
`, streamName, rInt) | ||
} | ||
|
||
func fulfillSleep() resource.TestCheckFunc { |
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 put in a sleep here because I found that the trust policy for the IAM role needs to propagate before referencing the role in the tests
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 @kl4w 👋 We prefer that the resource usually handles this type of logic by retrying on the specific error message(s) that are returned for a minute or two. e.g.
// Handle IAM eventual consistency
err := resource.Retry(1*time.Minute, func() *resource.RetryError {
// CreateApplication as an example
err := conn.CreateApplication(input)
if err != nil {
// kinesisanalytics.ErrCodeInvalidArgumentException as an example
if isAWSErr(err, kinesisanalytics.ErrCodeInvalidArgumentException, "some IAM-related error message, e.g. not authorized") {
return resource.RetryableError(err)
}
return resource.NonRetryableError(err)
}
return nil
})
if err != nil {
return fmt.Errorf("error creating application: %s", err)
}
vendor/vendor.json
Outdated
@@ -750,6 +750,14 @@ | |||
"version": "v1.14.33", | |||
"versionExact": "v1.14.33" | |||
}, | |||
{ | |||
"checksumSHA1": "nUdxsOW9jg+m+sWZYPu7oX9rZo8=", | |||
"path": "github.com/aws/aws-sdk-go/service/kinesisanalytics", |
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 you please separate the vendoring into its own PR? We do this because these dependencies are a fast moving target, it makes the PRs a little more manageable, and we can quickly merge it in. FYI master
is already AWS SDK Go v1.15.4
.
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.
Absolutely. I've created the PR here: #5461
8e6da57
to
3bc0d53
Compare
3bc0d53
to
193b409
Compare
|
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.
Phew, @kl4w, what a complicated service and resource! 😅 Great work so far. Please see below for some initial feedback and please reach out if you have any questions or do not have time to implement the items. 👍
} | ||
resp, err := conn.DescribeApplication(describeOpts) | ||
if err != nil { | ||
if awsErr, ok := err.(awserr.Error); ok { |
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: This error handling logic can be simplified with:
if isAWSErr(err, kinesisanalytics.ErrCodeResourceNotFoundException, "") {
log.Printf("[WARN] Kinesis Analytics Application (%d) not found, removing from state", d.Id())
d.SetId("")
return nil
}
if err != nil {
return fmt.Errorf("error reading Kinesis Analytics Application (%s): %s", d.Id(), err)
}
return err | ||
} | ||
|
||
d.SetId(aws.StringValue(resp.ApplicationDetail.ApplicationARN)) |
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.
d.SetId()
should be called during the Create
function unless the Update
function will change the ARN.
|
||
if v, ok := d.GetOk("cloudwatch_logging_options"); ok { | ||
clo := v.([]interface{})[0].(map[string]interface{}) | ||
cloudwatchLoggingOption := createCloudwatchLoggingOption(clo) |
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 project uses a very flat Go package structure, we prefer to include the AWS service name in the function name. We also generally call these functions "expand" functions. e.g. expandKinesisAnalyticsCloudwatchLoggingOption
Same for the below createX
function names.
d.Set("status", aws.StringValue(resp.ApplicationDetail.ApplicationStatus)) | ||
d.Set("version", int(aws.Int64Value(resp.ApplicationDetail.ApplicationVersionId))) | ||
|
||
if err := d.Set("cloudwatch_logging_options", getCloudwatchLoggingOptions(resp.ApplicationDetail.CloudWatchLoggingOptionDescriptions)); 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.
Since this project uses a very flat Go package structure, we prefer to include the AWS service name in the function name. We also generally call these functions "flatten" functions. e.g. flattenKinesisAnalyticsCloudwatchLoggingOptions
Same for the below getX
function names.
d.Set("version", int(aws.Int64Value(resp.ApplicationDetail.ApplicationVersionId))) | ||
|
||
if err := d.Set("cloudwatch_logging_options", getCloudwatchLoggingOptions(resp.ApplicationDetail.CloudWatchLoggingOptionDescriptions)); 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.
Nit: Its generally helpful to provide the operator (and code maintainers) some context about the error message they are receiving, e.g. return fmt.Errorf("error setting cloudwatch_logging_options: %s", err)
Same for below d.Set()
error handlers as well. 👍
|
||
func createCloudwatchLoggingOption(clo map[string]interface{}) *kinesisanalytics.CloudWatchLoggingOption { | ||
cloudwatchLoggingOption := &kinesisanalytics.CloudWatchLoggingOption{ | ||
LogStreamARN: aws.String(clo["log_stream"].(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.
We should prefer to stick to the API (and more clear) attribute naming for any of these ARN
parameters that accept an ARN, e.g. log_stream_arn
This applies to a large amount of the attributes below as well so I'll withhold from commenting on each of them.
testAccCheckKinesisAnalyticsApplicationExists(resName, &application), | ||
resource.TestCheckResourceAttr(resName, "version", "2"), | ||
resource.TestCheckResourceAttr(resName, "cloudwatch_logging_options.#", "1"), | ||
resource.TestMatchResourceAttr(resName, "cloudwatch_logging_options.0.log_stream", streamRe), |
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.
Instead of regexp matching the log stream ARN, you may find it easier to directly check the resource information via:
resource.TestCheckResourceAttrPair(resName, "cloudwatch_logging_options.0.log_stream_arn", "aws_cloudwatch_log_stream.test", "arn"),
This applies to the other below checks as well 😄
return fmt.Errorf("Error: Application still exists") | ||
} | ||
} | ||
return 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.
We should remove this return nil
to ensure this function can check all configured aws_kinesis_analytics_application
resources.
} | ||
resp, err := conn.DescribeApplication(describeOpts) | ||
if err == nil { | ||
if resp.ApplicationDetail != nil && *resp.ApplicationDetail.ApplicationStatus != kinesisanalytics.ApplicationStatusDeleting { |
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.
If the application does not delete immediately, should we be waiting for the deletion to complete in the Delete
function? 🤔
5f82d55
to
5e9b5b8
Compare
@bflad made changes as requested. test output:
|
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.
Great work, @kl4w! 🚀 Sorry for the delay in re-review after your updates! 😅 With such a complex resource like this is probably best to let folks start playing with it to tease out anything the testing missed -- LGTM though.
--- PASS: TestAccAWSKinesisAnalyticsApplication_basic (11.06s)
--- PASS: TestAccAWSKinesisAnalyticsApplication_update (17.23s)
--- PASS: TestAccAWSKinesisAnalyticsApplication_addCloudwatchLoggingOptions (25.38s)
--- PASS: TestAccAWSKinesisAnalyticsApplication_referenceDataSource (30.47s)
--- PASS: TestAccAWSKinesisAnalyticsApplication_updateCloudwatchLoggingOptions (38.74s)
--- PASS: TestAccAWSKinesisAnalyticsApplication_referenceDataSourceUpdate (54.39s)
--- PASS: TestAccAWSKinesisAnalyticsApplication_outputsKinesisStream (87.98s)
--- PASS: TestAccAWSKinesisAnalyticsApplication_inputsKinesisStream (88.00s)
--- PASS: TestAccAWSKinesisAnalyticsApplication_outputsAdd (94.81s)
--- PASS: TestAccAWSKinesisAnalyticsApplication_inputsAdd (110.65s)
--- PASS: TestAccAWSKinesisAnalyticsApplication_outputsUpdateKinesisStream (172.40s)
--- PASS: TestAccAWSKinesisAnalyticsApplication_inputsUpdateKinesisStream (172.42s)
This has been released in version 1.43.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. |
Hello @kl4w @bflad. Thank you very much for this contribution! I created a At the moment I can go directly to the UI and click the button to start the application: I guess I can start the application using Terraform, maybe I have to configure that action or something else. Am I missing something? |
@arturoherrero you are correct, there's no functionality to start the application right now on creation. This probably could implemented by changing the |
Hi @kl4w, I'm trying to add an additional output for the The block in
But the corresponding block in the state file does not reflect that stream:
I'm wondering if there's something wrong in my configuration or it's not possible to create the connection to a stream created by Kinesis Analytics by default. Thanks! Using terraform 0.11.8 and aws provider 1.52 |
@netrebel there's nothing wrong with your configuration. For some reason, I didn't iterate over the list of outputs and only captures the first output. Can you please submit a new Issue if not yet created? |
@kl4w Before creating the issue I updated to the latest terraform (0.11.11) and aws provider (1.58) and when I ran it again and I got this in the log, which I had not seen before:
So I thought it had been fixed already, but looking at the AWS console I still don't see the connection to the I think I see what you mean with taking only the first element: func flattenKinesisAnalyticsOutputs(outputs []*kinesisanalytics.OutputDescription) []interface{} {
s := []interface{}{}
if len(outputs) > 0 {
id := outputs[0]
... I'll create the Issue. Thanks for the quick response! |
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! |
Still work in progress right now, still need to support
Outputs
: https://docs.aws.amazon.com/kinesisanalytics/latest/dev/API_CreateApplication.html#analytics-CreateApplication-request-OutputsFixes #536
Changes proposed in this pull request:
Output from acceptance testing: