- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 9
remove deprecated s3 settings from module #41
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
base: main
Are you sure you want to change the base?
remove deprecated s3 settings from module #41
Conversation
044bc74    to
    cd3f6e2      
    Compare
  
    | /terratest | 
| With this removal, we lose the  | 
| 
 Currently if you are using aws provider version 6.0.0, this module is broken because that s3_settings is no longer valid. I'll be happy to make adjustments to the PR but I don't follow what changes should be made. Here is where my head is at: If we leave it in, anyone who continues to use the latest versions of the aws provider can not use this module, if we remove  I don't see a way that we can have this module functional for those who are using the most current aws provider unless it's removed, but like I said I'm very happy to make any adjustments to get this fixed | 
| } | ||
| } | ||
|  | ||
| dynamic "s3_settings" { | 
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 removing this functionality, could we migrate the s3_settings to the resource?
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 removing this functionality, could we migrate the
s3_settingsto the resource?
I'm trying to think this through, and want to make it clear that I know sometimes in PR Reviews through text ppl can sound defensive or argumentative, so I want to say that I really appreciate your question and I'm just hoping we find a great solution to this :) And if we go with your suggestion, I'd just appreciate any additional context for the sort of change you're looking for
My first thought is that this module is aws-dms-endpoint which is a specific terraform resource, and aws-dms-s3-endpoint is a different terraform resource, so I'm not sure it makes sense to have both of these resources in a module called aws-dms-endpoint. I could see the creation of a new module for aws-dms-s3-endpoint though.
Second thought is if the users of this module had set extra_connection_attributes, that is no longer supported with the new resource and this will also cause a break in their code.
Third thought is about the logistics of behind the scenes destroying and creating a new resource for users. If we double up in this module and include both resources, I'm not sure if simply removing s3_settings would destroy the old endpoint, or if it would persist, and then another endpoint resource would attempt to be created. If that were to happen, I am wondering if there would be a name conflict since they'd both have the same name. Also, if this did work, I'm not sure users would be pleased to have their endpoint destroyed and a new one created, if this happened in production to someone, it could interrupt their service.
Last thought is to the opposite of this, which is how much more or less complicated/disruptive would it be if instead of going this route, we did the following:
- remove the deprecated/invalid block from the resource
- cut a new release that is v2.0.0 with this change
If we go this route, then all users that wish to continue to use the deprecated s3_settings block can by staying at version v1.3.1 and they can migrate to the non-deprecated newer aws_dms_s3_endpoint resource at a time of their own choosing, and everyone who is not using deprecated code can upgrade to v2.0.0
Maybe I'm misunderstanding though, is the idea more "let's include the new endpoint type in this module at a v2.0.0 and let users upgrade & change to the new resource at their convenience"?
what
Remove the
s3_settingsblock & variable from the modulewhy
Terraform has completely deprecated and remove s3_settings from the aws_dms_endpoint resource and now any builds result in an error of:
references
Latest version showing that s3_settings is no longer there:
https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/dms_endpoint
Previous version showing that it is there but should be removed from code because they will remove it themselves soon:
https://registry.terraform.io/providers/hashicorp/aws/5.100.0/docs/resources/dms_endpoint