-
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
Added missing s3_settings attributes #11576
Conversation
…ed API changes in aws-sdk-go. This is so that we can avoid using extra_connection_attributes.
Hi Folks, is there something that is left to do or can we merge this? Note that even though the size estimation label says "size/L", it is actually a trivial change. The LOC is high due to the number of additional attributes that were added. |
Hi @lyle-nel, I was happy to see this PR since we were also facing this issue. I tested it, but do run into a problem. I have defined the end point as:
This provisions fine with the 2.44.0 version of the AWS provider, but when I provision it with your version of the AWS provider, testing of the endpoint by AWS fails with the message: "Error Details: [message=Exception while converting from replicate model object Test endpoint, errType=, status=0, errMessage=, errDetails=]" Enabling the the commented lines ( When doing a Using PR version of aws provider:
Using 2.44.0 version:
Using Terraform 0.12.19 Am I missing something? Regards, |
Hi @gerovermaas, I will look into this tomorrow morning (UTC+2) and get back to you as soon as possible. |
@gerovermaas Thanks I am busy looking into why these additional attributes are showing up. Even in the 2.44.0 version in your example the EnableStatistics attribute is showing up even though that should only happen in the case of writing to parquet. I have tracked down one potential avenue where the defaults are set to empty strings, but taking that away does not have an effect on the outcome. I will keep you updated if I learn anything useful. For now, I just need to exclude the possibility that it is anything terraform related, then I can move on to seeing if the issue lies with the aws-go-sdk |
@lyle-nel Any progress or update on this PR? |
@gerovermaas I am actively working on this. I don't have much of an update besides that I have written some integration tests in the aws provider that exposes the issue. I am currently writing some tests for the schema helpers in terraform core. It is very unlikely that the issue is there, but I just need to exclude that possibility. For the sake of transparency, I will summarise the symptom: Consider the following resource schema: Schema: map[string]*schema.Schema{
"s3_settings": {
Type: schema.TypeList,
Optional: true,
MaxItems: 1,
DiffSuppressFunc: func(k, old, new string, d *schema.ResourceData) bool {
if old == "1" && new == "0" {
return true
}
return false
},
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"compression_type": {
Type: schema.TypeString,
Optional: true,
Default: "NONE",
},
"timestamp_column_name": {
Type: schema.TypeString,
Optional: true,
}
},
},
},
}, It is clear from this that |
TLDR; I need some help with fixing one of the test statements to get this over the finish line. @gerovermaas I managed to track down the issue. My tests led me astray, so the solution is pretty straightforward. The main issue I am faced with now is to get the acceptance test to pass. I know the changes work because when I view the response object, only the desired fields are set. I am using Test output:
The response object from the create endpoint routine is as follows:
|
Great to read @lyle-nel ! |
12c80af
to
8a84842
Compare
I am using |
I have tested the version and can confirm that it now provision correct and I'm able to generate parquet files. Used the following resource definition:
And
Would be great if this PR can be merged, it would help us a lot! |
This would also help me out quite a bit. Thank you so much @lyle-nel for all the hard work! |
We have been doing some more testing and although the
And
When we do an additional
Is this something you can look into @lyle-nel ? |
…ss it has a default or is explicitly populated.
@gerovermaas Yes, I know what the problem is. My sincere apologies for this folks, once I understand what I am doing wrong with this one test, the test would expose issues such as this. I am hoping to figure out how this part needs to work soon, but some guidance from someone with experience in developing providers would go a long way. In the meanwhile, I have pushed the fix. @gerovermaas , thanks for your valuable feedback and continued patience. |
Thank @lyle-nel ! I tested the new version and now the Used this definition and I'm happy we do not need the
|
@bflad I see that you are one of the more recent contributors to this component. I was hoping you could have a quick look at this? Am I missing something very obvious that is causing the terraform state to contain the optional attributes even though they are not being set? |
I am closing this pull request until I can figure out how to fix the test. |
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! |
Extend DMS S3 settings to support updated API changes in aws-sdk-go. This is so that we can avoid using extra_connection_attributes.
Full list of supported attributes for s3_settings were found here: models/apis/dms/2016-01-01/api-2.json
Community Note
Relates OR Closes #8000 #8009
Release note for CHANGELOG:
ENHANCEMENTS:
s3_settings
attributesServiceAccessRoleArn, ExternalTableDefinition, CsvRowDelimiter, CsvDelimiter, BucketFolder, BucketName, CompressionType, TimestampColumnName, DataFormat, ParquetVersion, EncryptionMode, ServerSideEncryptionKmsKeyId
Output from acceptance testing: