Working example for Atlas-encryptionAtRest-roles with a single tf apply #415
Conversation
|
@shum @leofigy @coderGo93 can you all let me know your thoughts? A nice PR from one of our CSAs! |
| curl --user "${var.public_key}:${var.private_key}" -X PATCH --digest \ | ||
| --header "Accept: application/json" \ | ||
| --header "Content-Type: application/json" \ | ||
| "https://cloud.mongodb.com/api/atlas/v1.0/groups/${var.project_id}/cloudProviderAccess/${mongodbatlas_cloud_provider_access.test.role_id}?pretty=true" \ |
|
Not a TF expert so can't comment on whether this approach is "best practice™️" , but this is a nifty hack to workaround the problem and looks good to me! |
|
@zohar-mongo thank you again for knocking this out. Instead of changing the original example I think we should consider spliting it into two. Keep the original as is (though w/ the bug fix here for the unused variables fyi @coderGo93) but actually make it two directories, one called examples/atlas-encryptionAtRest-roles-two-step and examples/atlas-encryptionAtRest-roles-one-step-workaround. Then if we get it to one step without a workaround we add that one. Would you mind doing the rework on that? |
|
@themantissa Sure |
|
@themantissa Updated the pull request per your request :) |
There was a problem hiding this comment.
Looks good, thank you for knocking this out. There's one artifact (the second-step 2nd directory in the one step example) but we can fix that up post merge. I'll have one of our devs take a look again and double check but this should help a lot for now. Thanks!
|
@coderGo93 or @leofigy can one of you give one more look? I think we'll need to clean up one of the second-step directories (in the copy from the current example) for the one step workaround but we can do that later I think. |
|
@themantissa Thanks, I didn't modify the "second step" folder. From what I understand, this is the next step for configuring the encryption at rest with the role_id we exported in the first step (creating the role_id with the correct iam_assumed_role_arn in one step was the problem) |
|
Ah, I follow. We could move the second into one big example I believe @zohar-mongo but we can improve later. Just waiting for one more review from those selected. Thanks! |
| } | ||
| mongodbatlas = { | ||
| source = "mongodb/mongodbatlas" | ||
| //version = "0.7-dev" |
There was a problem hiding this comment.
just could you remove this comment from here
|
@nikhil-mongo I'm going to merge this soon but it could use some clean-up potentially. Can you take a look and let me know if you have any concerns before I merge? |
| @@ -0,0 +1,76 @@ | |||
|
|
|||
There was a problem hiding this comment.
Please remove this extra line.
| @@ -0,0 +1,33 @@ | |||
| variable "public_key" { | |||
| description = "The public API key for MongoDB Atlas" | |||
| default = "" | |||
There was a problem hiding this comment.
The default values passed as an empty string just add more lines and are of no use. @themantissa if you think we can keep it, its ok. Else please remove these as well.
| @@ -0,0 +1,36 @@ | |||
| variable "public_key" { | |||
| description = "The public API key for MongoDB Atlas" | |||
| default = "" | |||
There was a problem hiding this comment.
The default values passed as an empty string just add more lines and are of no use. @themantissa if you think we can keep it, its ok. Else please remove these as well.
There was a problem hiding this comment.
LGTM. @themantissa if you want those variables to be kept or not, I'm good with both. But this is good to be merged.
|
@nikhil-mongo thank you. We can keep for now and if you'd like to do a bit of clean up later it would be appreciated. Thank you. |
Description
Working example for Atlas-encryptionAtRest-roles with a single
terraform apply. This is a workaround that overcomes the cycle issue. The existing example has a bug where theiam_assumed_role_arnargument is populated with thevar.aws_iam_role_arnvariable, which has a default of an empty string.The
output "aws_iam_role_arn"was removed since it's not in use, and it creates confusion.Link to any related issue(s):
Type of change:
Required Checklist:
Further comments