Skip to content
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

DMS Endpoint: Apply extra_connection_attributes to all engine types in DMS #5607

Closed

Conversation

microamp
Copy link
Contributor

Changes proposed in this pull request:

  • For certain DMS endpoint engines, there is a dedicated set of the engine-specific settings. s3_settings for s3 and mongodb_settings for mongodb to name a few. However, there exist engine-specific attributes that can only be changed via extra_connection_attributes. While this appears to be an inconsistent behaviour of the corresponding AWS API, we have to make extra_connection_attributes available to all engine types. One example is addColumnName for s3. It's currently not being handled under s3_settings hence must be specified in extra_connection_attributes if you want a non-default behaviour (in this case, false).

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAwsDmsEndpointS3'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run=TestAccAwsDmsEndpointS3 -timeout 120m
?      github.com/terraform-providers/terraform-provider-aws  [no test files]
=== RUN   TestAccAwsDmsEndpointS3
--- PASS: TestAccAwsDmsEndpointS3 (81.30s)
PASS
ok     github.com/terraform-providers/terraform-provider-aws/aws      81.309s

@ghost ghost added the size/S Managed by automation to categorize the size of a PR. label Aug 20, 2018
@microamp microamp changed the title Apply extra_connection_attributes to all engine types in DMS DMS Endpoint: Apply extra_connection_attributes to all engine types in DMS Aug 20, 2018
@microamp
Copy link
Contributor Author

microamp commented Aug 27, 2018

Hi. Do we have any updates on this by any chance? At present, any non-default extra_connection_attributes for s3 and mongodb are not going through when the task is first created due to this bug. Something I should've mentioned earlier. 😺

@stephencoe
Copy link
Contributor

I have also come across this issue recently, currently the only way to get the extra_connection_attributes for services is to trigger an update and double apply 👎 It would be good to see this merged soon

@@ -279,14 +279,14 @@ func resourceAwsDmsEndpointCreate(d *schema.ResourceData, meta interface{}) erro
if v, ok := d.GetOk("database_name"); ok {
request.DatabaseName = aws.String(v.(string))
}
if v, ok := d.GetOk("extra_connection_attributes"); ok {
request.ExtraConnectionAttributes = aws.String(v.(string))
}
if v, ok := d.GetOk("kms_key_arn"); ok {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While it's not related to extra_connection_attributes I also think the kms_key_arn is in the wrong place here.

Its handled in the state outside of the default block and the docs describe this as the encryption key used by the replication instance and connection information so it shouldn't be service specific.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing that out. I'll take a closer look at it soon.

@microamp microamp force-pushed the dms-endpoint-extra-conn-attrs branch from b933577 to 36b9617 Compare September 11, 2018 04:10
@ghost ghost added the size/S Managed by automation to categorize the size of a PR. label Sep 11, 2018
@microamp
Copy link
Contributor Author

@bflad @stephencoe Rebased and placed two commits on top. Please review/test.

@ghost
Copy link

ghost commented Sep 26, 2018

I have just been caught out by this. I had set extra_connection_attributes = "addColumnName=true" Set this about a month ago, about to write a lambda to leverage the column headers and they aren't to be found in the S3 files.

Any update on when this will be merged?

@a4c-ricardo-chaves
Copy link

I have the same issue.

@ramziyassine
Copy link

I ran into the same issue will this be something mergeable in Terraform 12?

@aeschright aeschright requested a review from a team June 25, 2019 21:27
@elijah-ward
Copy link

Same issue 😪. I'm wondering since this has been open for so long, have people found a workaround? I'm wanting to set DataFormat: parquet on an s3 target endpoint in addition to some of the other extra params mentioned above.

@anGie44
Copy link
Contributor

anGie44 commented Jan 12, 2021

Hi @microamp, thank you for you submission and apologies for the wait! given the changes in the codebase since the time of your submission we'll be moving forward with #16827 to address the extra_connection_attributes change to make it applicable to all engine types.

@anGie44 anGie44 closed this Jan 12, 2021
@ghost
Copy link

ghost commented Feb 12, 2021

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!

@ghost ghost locked as resolved and limited conversation to collaborators Feb 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
size/S Managed by automation to categorize the size of a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants