r/aws_lambda_event_source_mapping: Infer enabled status from State atttribute

[flosell]

Fixes #498

Changes proposed in this pull request:

• This PR adds logic to map LambdaEventSourceMappings State attribute to the resources enabled parameter to allow terraform to detect if the enabled parameter was changed outside of terraforms control and change it back.

Open question:

• State can have lots of values, Creating, Enabled, Disabled, Enabling, Disabling, Updating, or Deleting. Currently, this PR only handles the clear cases Enabled and Disabled. In all other cases we just ignore the output and keep the terraform state as it is.
  Is this a problem that should be adressed in this PR? If yes, does it make more sense to "guess" the future state (e.g. Enabling will become Enabled) or wait and retry until the state has converged?

Output from acceptance testing:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSLambdaEventSourceMapping'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSLambdaEventSourceMapping -timeout 120m
=== RUN   TestAccAWSLambdaEventSourceMapping_kinesis_basic
--- PASS: TestAccAWSLambdaEventSourceMapping_kinesis_basic (156.59s)
=== RUN   TestAccAWSLambdaEventSourceMapping_kinesis_removeBatchSize
--- PASS: TestAccAWSLambdaEventSourceMapping_kinesis_removeBatchSize (147.12s)
=== RUN   TestAccAWSLambdaEventSourceMapping_sqs_basic
--- PASS: TestAccAWSLambdaEventSourceMapping_sqs_basic (156.78s)
=== RUN   TestAccAWSLambdaEventSourceMapping_kinesis_import
--- PASS: TestAccAWSLambdaEventSourceMapping_kinesis_import (120.90s)
=== RUN   TestAccAWSLambdaEventSourceMapping_kinesis_disappears
--- PASS: TestAccAWSLambdaEventSourceMapping_kinesis_disappears (118.96s)
=== RUN   TestAccAWSLambdaEventSourceMapping_sqsDisappears
--- PASS: TestAccAWSLambdaEventSourceMapping_sqsDisappears (154.77s)
=== RUN   TestAccAWSLambdaEventSourceMapping_changesInEnabledAreDetected
--- PASS: TestAccAWSLambdaEventSourceMapping_changesInEnabledAreDetected (184.31s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	1039.497s

[bflad]

Hi @flosell! 👋 Thanks for submitting this drift detection fix! Nice work on the acceptance test. Overall this is looking really good and is almost ready for merge. I left two notes below.

To answer your questions specifically:

Is this a problem that should be adressed in this PR? If yes, does it make more sense to "guess" the future state (e.g. Enabling will become Enabled) or wait and retry until the state has converged?

Generally speaking, it only matters if not waiting for it to be fully enabled will have consequences downstream with other resources. The fix of detecting the drift is more important to get in. 😄 I provided an example implementation that allows both Enabling and Enabled to be okay for now. We can treat waiting for Enabling -> Enabled during update as a separate change if for some reason its necessary.

[bflad]

In this case, I would recommend something like:

state := aws.StringValue(eventSourceMappingConfiguration.State)
d.Set("enabled", (state == "Enabling" || state == "Enabled"))

The key here being that d.Set() is always called to ensure we are reading the value from the API into the Terraform state to detect configuration drift. The aws.StringValue() SDK function helps prevent potential panics should the State attribute be missing from the API.

[flosell]

Thanks for the pointer for StringValue! I had to be a bit more advanced for the enabled state though, otherwise it also maps Creating, Updating, Deleting to false which breaks the tests (and is also not what we want).

[bflad]

Nitpick: We generally prefer to use our helper function to simplify this logic:

if isAWSErr(err, lambda.ErrCodeResourceInUseException, "") {

[flosell]

Fixed, thanks for the pointer!

[flosell]

@bflad: Thanks for the feedback, should all be resolved.

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSLambdaEventSourceMapping'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSLambdaEventSourceMapping -timeout 120m
=== RUN   TestAccAWSLambdaEventSourceMapping_kinesis_basic
--- PASS: TestAccAWSLambdaEventSourceMapping_kinesis_basic (151.03s)
=== RUN   TestAccAWSLambdaEventSourceMapping_kinesis_removeBatchSize
--- PASS: TestAccAWSLambdaEventSourceMapping_kinesis_removeBatchSize (146.85s)
=== RUN   TestAccAWSLambdaEventSourceMapping_sqs_basic
--- PASS: TestAccAWSLambdaEventSourceMapping_sqs_basic (118.40s)
=== RUN   TestAccAWSLambdaEventSourceMapping_kinesis_import
--- PASS: TestAccAWSLambdaEventSourceMapping_kinesis_import (119.49s)
=== RUN   TestAccAWSLambdaEventSourceMapping_kinesis_disappears
--- PASS: TestAccAWSLambdaEventSourceMapping_kinesis_disappears (113.61s)
=== RUN   TestAccAWSLambdaEventSourceMapping_sqsDisappears
--- PASS: TestAccAWSLambdaEventSourceMapping_sqsDisappears (149.49s)
=== RUN   TestAccAWSLambdaEventSourceMapping_changesInEnabledAreDetected
--- PASS: TestAccAWSLambdaEventSourceMapping_changesInEnabledAreDetected (162.54s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	961.460s

[bflad]

Looks good, @flosell! 🚀

7 tests passed (all tests)
=== RUN   TestAccAWSLambdaEventSourceMapping_kinesis_disappears
--- PASS: TestAccAWSLambdaEventSourceMapping_kinesis_disappears (78.20s)
=== RUN   TestAccAWSLambdaEventSourceMapping_kinesis_import
--- PASS: TestAccAWSLambdaEventSourceMapping_kinesis_import (78.99s)
=== RUN   TestAccAWSLambdaEventSourceMapping_kinesis_removeBatchSize
--- PASS: TestAccAWSLambdaEventSourceMapping_kinesis_removeBatchSize (84.38s)
=== RUN   TestAccAWSLambdaEventSourceMapping_kinesis_basic
--- PASS: TestAccAWSLambdaEventSourceMapping_kinesis_basic (85.34s)
=== RUN   TestAccAWSLambdaEventSourceMapping_sqs_basic
--- PASS: TestAccAWSLambdaEventSourceMapping_sqs_basic (97.03s)
=== RUN   TestAccAWSLambdaEventSourceMapping_sqsDisappears
--- PASS: TestAccAWSLambdaEventSourceMapping_sqsDisappears (99.31s)
=== RUN   TestAccAWSLambdaEventSourceMapping_changesInEnabledAreDetected
--- PASS: TestAccAWSLambdaEventSourceMapping_changesInEnabledAreDetected (99.39s)

[bflad]

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

[ghost]

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!