INTMDB-243: Fixes a bug for encryption at rest with new parameters #522

coderGo93 · 2021-08-19T14:58:18Z

Description

Fixes a bug for encryption at rest when it would appear a difference of type
Fixes a behavior bug when after terraform plan appeared a diff even there's not changes because of parameter enabled setting with the default when it shouldn't
Fixes a bug when the deprecated parameters would appear changes after terraform plan even if you don't use them
Added terratest for Upgrade and create the encryption at rest using AWS/Azure/GCP

Tested manually with Aws, GCP and Azure credentials and it worked without appearing changes after terraform plan

Results of Terratest

Upgrades for AWS/GCP/Azure

--- PASS: TestUpgradeEncryptionAtRestAws (56.51s)
PASS
ok      github.com/mongodb/terraform-provider-mongodbatlas/integration-testing  56.519s

--- PASS: TestUpgradeEncryptionAtRestAzure (20.81s)
PASS
ok      github.com/mongodb/terraform-provider-mongodbatlas/integration-testing  20.819s


--- PASS: TestUpgradeEncryptionAtRestGCP (17.07s)
PASS
ok      github.com/mongodb/terraform-provider-mongodbatlas/integration-testing  17.081s

Create for AWS/GCP/Azure

--- PASS: TestResourceEncryptionAtRestAws (33.90s)
PASS
ok      github.com/mongodb/terraform-provider-mongodbatlas/integration-testing  33.914s


--- PASS: TestResourceEncryptionAtRestAzure (11.11s)
PASS
ok      github.com/mongodb/terraform-provider-mongodbatlas/integration-testing  11.120s


--- PASS: TestResourceEncryptionAtRestGCP (11.24s)
PASS
ok      github.com/mongodb/terraform-provider-mongodbatlas/integration-testing  11.251s

Link to any related issue(s):

Type of change:

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update
Documentation fix/enhancement

Required Checklist:

I have signed the MongoDB CLA
I have read the Terraform contribution guidelines
I have added tests that prove my fix is effective or that my feature works per HashiCorp requirements
I have added any necessary documentation (if appropriate)
I have run make fmt and formatted my code

Further comments

thetonymaster

Just some nitpicks

thetonymaster · 2021-08-23T20:32:59Z

examples/test-upgrade/encryption-at-rest/azure/v091/main.tf

+    azure_environment   = "AZURE"
+    subscription_id     = var.subscription_id
+    resource_group_name = var.resource_group_name
+    key_vault_name  	= var.key_vault_name


can we properly format this?

thetonymaster · 2021-08-23T20:33:13Z

examples/test-upgrade/encryption-at-rest/azure/v101/main.tf

+    key_identifier  	= var.key_identifier
+    secret  			= var.client_secret
+    tenant_id  			= var.tenant_id
+  }


this also needs formatting

thetonymaster · 2021-08-23T20:33:26Z

examples/test-upgrade/encryption-at-rest/gcp/v091/main.tf

+resource "mongodbatlas_encryption_at_rest" "test" {
+  project_id = mongodbatlas_project.test.id
+
+  google_cloud_kms = {


format this too

gssbzn

Left a couple of questions, let's also format those examples as has been requested, I'm merging #529 soon to make sure they are always

gssbzn · 2021-08-25T16:23:48Z

integration-testing/resource_mongodbatlas_encryption_at_rest_test.go

@@ -1,12 +1,12 @@
-// +build integration


[q] I've been setting up the repo thru the day and going with docs code, is the plan to stop taging integration tests? it seems some are using it and some are not, can we decide on this and updates docs accordingly (some of the docs on how to test reference the use of tags)?

Ok, forgot to un comment that part, I have to make another PR with fork because I cannot make changes and push

@coderGo93 can you address this now?

I'm not sure why was added this line but it seems better to delete this line because in GNUMakefile it's already separating from normal tests with make test, the other similar line is in the resource_mongodbatlas_cloud_provider_access_test.go from integrationtesting folder, should I remove the line as well in this PR? @themantissa

I'm not entirely sure, if you feel it should be removed please do.

understood and removed, thank you melissa

gssbzn · 2021-08-25T16:29:22Z

mongodbatlas/resource_mongodbatlas_encryption_at_rest.go

@@ -323,19 +322,22 @@ func resourceMongoDBAtlasEncryptionAtRestRead(ctx context.Context, d *schema.Res
 	if len(values2) != 0 {
 		value := values2[0].(map[string]interface{})
 		if !counterEmptyValues(value) {
-			aws, awsOk := d.GetOk("aws_kms")
+			aws, awsOk := d.GetOk("aws_kms_config")


line 280 says aws_kms are both different things? what's the difference?

The difference between aws_kms and aws_kms_config is the schema, I have to changed that because in new tf it fails to pass the make test because using TypeMap as object is not allowed, it should be used as TypeList with Max items 1

gssbzn · 2021-08-25T16:48:00Z

mongodbatlas/resource_mongodbatlas_encryption_at_rest.go

@@ -177,7 +177,7 @@ func resourceMongoDBAtlasEncryptionAtRest() *schema.Resource {
 					Schema: map[string]*schema.Schema{
 						"enabled": {
 							Type:     schema.TypeBool,
-							Optional: true,
+							Required: true,


[q] would this be a breaking change if I did not set it?

I changed to Required because I re read the docs and the parameter enabled is required

it's probably a silly question from me just making sure if people where not setting it on their files (as it was optional) will start having errors now that's required

Good point, will rollback to optional then, thank you gustavo

coderGo93 · 2021-08-25T18:38:18Z

Will close this PR to create another PR

themantissa

LGTM, will merge.

coderGo93 self-assigned this Aug 19, 2021

coderGo93 requested review from abner-dou and andreaangiolillo as code owners August 19, 2021 14:58

coderGo93 requested review from themantissa and thetonymaster August 19, 2021 14:58

thetonymaster approved these changes Aug 23, 2021

View reviewed changes

themantissa requested review from a team and gssbzn and removed request for andreaangiolillo and a team August 24, 2021 20:30

gssbzn approved these changes Aug 25, 2021

View reviewed changes

coderGo93 closed this Aug 25, 2021

coderGo93 mentioned this pull request Aug 25, 2021

INTMDB-243: Fixes a bug for encryption at rest with new parameters #531

Closed

10 tasks

coderGo93 reopened this Aug 25, 2021

Edgar Lopez added 5 commits August 25, 2021 14:14

refactor: fixes behavior bugs

7d02f58

refactor: deleted diff supress because is not necessary

790c2f4

added terratest

22fc574

refactor: run terraform fmt

3f2e9f9

fixes linter

bf76bf5

coderGo93 force-pushed the INTMDB-243 branch from 449d0ec to bf76bf5 Compare August 25, 2021 20:19

deleted build integration in cloud provider access

0b28049

themantissa approved these changes Aug 25, 2021

View reviewed changes

coderGo93 added the run-testacc To run acceptance tests label Aug 26, 2021

rollback to optional in gcp kms enabled

dfd482b

themantissa merged commit 28c94b0 into master Aug 26, 2021

themantissa deleted the INTMDB-243 branch August 26, 2021 19:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

INTMDB-243: Fixes a bug for encryption at rest with new parameters #522

INTMDB-243: Fixes a bug for encryption at rest with new parameters #522

coderGo93 commented Aug 19, 2021 •

edited

thetonymaster left a comment

thetonymaster Aug 23, 2021

thetonymaster Aug 23, 2021

thetonymaster Aug 23, 2021

gssbzn left a comment

gssbzn Aug 25, 2021

coderGo93 Aug 25, 2021

themantissa Aug 25, 2021

coderGo93 Aug 25, 2021

themantissa Aug 25, 2021

coderGo93 Aug 25, 2021

gssbzn Aug 25, 2021

coderGo93 Aug 25, 2021

gssbzn Aug 25, 2021

coderGo93 Aug 25, 2021

gssbzn Aug 26, 2021

coderGo93 Aug 26, 2021

coderGo93 commented Aug 25, 2021

themantissa left a comment

INTMDB-243: Fixes a bug for encryption at rest with new parameters #522

INTMDB-243: Fixes a bug for encryption at rest with new parameters #522

Conversation

coderGo93 commented Aug 19, 2021 • edited

Description

Type of change:

Required Checklist:

Further comments

thetonymaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gssbzn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coderGo93 commented Aug 25, 2021

themantissa left a comment

Choose a reason for hiding this comment

coderGo93 commented Aug 19, 2021 •

edited