-
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
Fix change detection relating to vpc_config #17610
Conversation
@gdavison I'd appreciate some feedback on this to see if it is an appropriate approach to work around the issue in the dependant package and also some advice on the approach to adding some unit tests for this. I just noticed there is a PR already open trying to do the exact same thing. Is there any chance either of them could be reviewed/merged as the related issue is quite impacting |
I've added acceptance tests to cover this as I couldn't work out how to add unit tests for it. However, the tests fail as the plan shows the qualified_arn and version fields will be computed (See below). If I add ExpectNonEmptyPlan: true then the test does correctly show a new version is not published as expected.
|
As reported in #17385 a change i detected in vpc_config when there are no changes, this is caused by an issue in hashicorp/terraform-plugin-sdk#617 The PR to provide a fix is not getting any traction. On further debuging the issue is caused by the nested elements being of type set which needs the use of Equal rather than reflect.DeepEqual to test for differences. We can work around this bug by testing for changes in the two fields within vpc_config independantly as when the item passed to HasChanges is a Set it is tested correctly.
Thank you for your contribution! 🚀 Please note that typically Go dependency changes are handled in this repository by dependabot or the maintainers. This is to prevent pull request merge conflicts and further delay reviews of contributions. Remove any changes to the Additional details:
|
Thank you for your contribution! 🚀 Please note that the Remove any changes to the |
This is now working but there is a failure in the acceptence test I added that makes sure a new version is published when the vpc config changes. The failure in the test seems to be due to the fact one of the two ENI's never get released so the associated security group can't delete. My assumption is this is related to the previously published version not being deleted correctly or that this doesn't break the association to the ENI (See below). I'm unsure how I can proceed with this now
|
Refactoring the test to remove the vpc config works around the above issue === RUN TestAccAWSLambdaFunctionEventInvokeConfig_basic Test ignored. Test ignored. |
Removing the VPC config is still a valid vpc config change and works around the ENI deltion issue.
any chance to get this in? |
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.
Looks good 🚀
--- PASS: TestAccAWSLambdaFunctionEventInvokeConfig_disappears_LambdaFunction (59.65s)
--- PASS: TestAccAWSLambdaFunctionEventInvokeConfig_disappears_LambdaFunctionEventInvokeConfig (66.23s)
--- PASS: TestAccAWSLambdaFunction_basic (68.01s)
--- PASS: TestAccAWSLambdaFunctionEventInvokeConfig_Qualifier_Latest (74.47s)
--- PASS: TestAccAWSLambdaFunctionEventInvokeConfig_FunctionName_Arn (87.93s)
--- PASS: TestAccAWSLambdaFunction_disappears (94.90s)
--- PASS: TestAccAWSLambdaFunctionEventInvokeConfig_basic (99.10s)
--- PASS: TestAccAWSLambdaFunction_expectFilenameAndS3Attributes (40.51s)
--- PASS: TestAccAWSLambdaFunctionEventInvokeConfig_Qualifier_FunctionVersion (102.81s)
--- PASS: TestAccAWSLambdaFunctionEventInvokeConfig_Qualifier_FunctionName_Arn (103.54s)
--- PASS: TestAccAWSLambdaFunctionEventInvokeConfig_Qualifier_AliasName (104.25s)
--- PASS: TestAccAWSLambdaFunction_nilDeadLetterConfig (33.89s)
--- PASS: TestAccAWSLambdaFunctionEventInvokeConfig_DestinationConfig_Remove (144.65s)
--- PASS: TestAccAWSLambdaFunction_concurrency (145.29s)
--- SKIP: TestAccAWSLambdaFunction_imageConfig (0.89s)
--- PASS: TestAccAWSLambdaFunctionEventInvokeConfig_MaximumEventAgeInSeconds (148.40s)
--- PASS: TestAccAWSLambdaFunction_UnpublishedCodeUpdate (149.49s)
--- PASS: TestAccAWSLambdaFunctionEventInvokeConfig_DestinationConfig_Swap (149.77s)
--- PASS: TestAccAWSLambdaFunction_Environment_Variables_NoValue (92.51s)
--- PASS: TestAccAWSLambdaFunctionEventInvokeConfig_DestinationConfig_OnSuccess_Destination (163.70s)
--- PASS: TestAccAWSLambdaFunctionEventInvokeConfig_DestinationConfig_OnFailure_Destination (173.54s)
--- PASS: TestAccAWSLambdaFunction_versioned (92.87s)
--- PASS: TestAccAWSLambdaFunctionEventInvokeConfig_MaximumRetryAttempts (194.46s)
--- PASS: TestAccAWSLambdaFunction_concurrencyCycle (194.58s)
--- PASS: TestAccAWSLambdaFunction_codeSigningConfig (199.96s)
--- PASS: TestAccAWSLambdaFunction_encryptedEnvVariables (135.93s)
--- PASS: TestAccAWSLambdaFunction_disablePublish (125.24s)
--- PASS: TestAccAWSLambdaFunction_Layers (78.37s)
--- PASS: TestAccAWSLambdaFunction_DeadLetterConfig (126.09s)
--- PASS: TestAccAWSLambdaFunction_DeadLetterConfigUpdated (127.94s)
--- PASS: TestAccAWSLambdaFunction_KmsKeyArn_NoEnvironmentVariables (85.17s)
--- PASS: TestAccAWSLambdaFunction_enablePublish (147.22s)
--- PASS: TestAccAWSLambdaFunction_versionedUpdate (157.89s)
--- PASS: TestFlattenLambdaImageConfigShouldNotFailWithEmptyImageConfig (0.00s)
--- PASS: TestAccAWSLambdaFunction_tracingConfig (108.73s)
--- PASS: TestAccAWSLambdaFunction_EmptyVpcConfig (58.25s)
--- PASS: TestAccAWSLambdaFunction_LayersUpdate (108.42s)
--- PASS: TestAccAWSLambdaFunction_s3 (51.81s)
--- PASS: TestAccAWSLambdaFunction_envVariables (197.77s)
--- PASS: TestAccAWSLambdaFunction_s3Update_basic (57.60s)
--- PASS: TestAccAWSLambdaFunction_localUpdate (64.50s)
--- PASS: TestAccAWSLambdaFunction_localUpdate_nameOnly (66.21s)
--- PASS: TestAccAWSLambdaFunction_s3Update_unversioned (62.70s)
--- PASS: TestAccAWSLambdaFunction_tags (73.68s)
--- PASS: TestAccAWSLambdaFunction_VPC (266.70s)
--- PASS: TestAccAWSLambdaFunction_VPC_withInvocation (257.83s)
--- PASS: TestAccAWSLambdaFunction_VpcConfig_ProperIamDependencies (268.26s)
--- PASS: TestAccAWSLambdaFunction_runtimes (327.80s)
--- PASS: TestAccAWSLambdaFunction_VPC_publish_No_Changes (441.95s)
--- PASS: TestAccAWSLambdaFunction_VPC_publish_Has_Changes (519.38s)
--- PASS: TestAccAWSLambdaFunction_VPCRemoval (1603.57s)
--- PASS: TestAccAWSLambdaFunction_VPCUpdate (1795.10s)
--- PASS: TestAccAWSLambdaFunction_FileSystemConfig (1953.35s)
This functionality has been released in v3.45.0 of the Terraform AWS Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you! |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
As reported in #17385 a change is detected in vpc_config when there are no changes, this is caused by an issue in hashicorp/terraform-plugin-sdk#617. The PR to provide a fix is not getting any traction.
On further debuging the issue is caused by the nested elements within the vpc_config stuct being of
type set which needs the use of Equal rather than reflect.DeepEqual to
test for differences.
We can work around this bug by testing for changes in the two fields
within vpc_config struct independantly as when the item passed to HasChanges is
a Set it is tested correctly.
I couldn't find a quick way to add tests for this, so I will work on this while the PR is in draft state
Community Note
Relates OR Closes #0000
Output from acceptance testing: