Add Backup Support to `azurerm_redis_cache` #130

tombuildsstuff · 2017-06-26T14:41:29Z

Adds support for the backup fields:

rdb-backup-enabled
rdb-backup-frequency
rdb-backup-max-snapshot-count
rdb-storage-connection-string

Fixes #75

tombuildsstuff · 2017-06-26T15:16:22Z

Tests pass:

$ TF_ACC=1 envchain azurerm go test ./azurerm -v -timeout 300m -parallel 5 -run TestAccAzureRMRedisCache_
=== RUN   TestAccAzureRMRedisCache_basic
--- PASS: TestAccAzureRMRedisCache_basic (636.27s)
=== RUN   TestAccAzureRMRedisCache_standard
--- PASS: TestAccAzureRMRedisCache_standard (747.48s)
=== RUN   TestAccAzureRMRedisCache_premium
--- PASS: TestAccAzureRMRedisCache_premium (627.24s)
=== RUN   TestAccAzureRMRedisCache_premiumSharded
--- PASS: TestAccAzureRMRedisCache_premiumSharded (1356.70s)
=== RUN   TestAccAzureRMRedisCache_NonStandardCasing
--- PASS: TestAccAzureRMRedisCache_NonStandardCasing (682.86s)
=== RUN   TestAccAzureRMRedisCache_BackupDisabled
--- PASS: TestAccAzureRMRedisCache_BackupDisabled (685.17s)
=== RUN   TestAccAzureRMRedisCache_BackupEnabled
--- PASS: TestAccAzureRMRedisCache_BackupEnabled (893.95s)
PASS

tombuildsstuff · 2017-06-26T17:15:20Z

Removed the State Migration and added a test enabling, then disabling the backup:

$ TF_ACC=1 envchain azurerm go test ./azurerm -v -timeout 300m -parallel 5 -run TestAccAzureRMRedisCache_Backup

=== RUN   TestAccAzureRMRedisCache_BackupDisabled
--- PASS: TestAccAzureRMRedisCache_BackupDisabled (650.91s)
=== RUN   TestAccAzureRMRedisCache_BackupEnabled
--- PASS: TestAccAzureRMRedisCache_BackupEnabled (645.86s)
=== RUN   TestAccAzureRMRedisCache_BackupEnabledDisabled
--- PASS: TestAccAzureRMRedisCache_BackupEnabledDisabled (694.48s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	1991.281s

radeksimko

I'm not overly happy about the types. I don't know what's considered conventional in MSFT/Azure world nor I have checked if this is what we do in other Azure resources, nonetheless I think it's defeating the purpose of having typed schema (and taking advantage of basic HCL/HIL validation).

radeksimko · 2017-06-27T12:26:01Z

azurerm/resource_arm_redis_cache.go

+							Type:             schema.TypeString,
+							Optional:         true,
+							DiffSuppressFunc: ignoreCaseDiffSuppressFunc,
+							ValidateFunc: validation.StringInSlice([]string{


Is there any particular reason we want to have string here? Stringifyed boolean values don't look nice and break the whole point of having a (semi)typed schema IMO.

Even if the reason here is API/SDK then I think we should solve that problem and do our bit internally in the CRUD and/or DiffSuppressFunc/StateFunc so that the user doesn't have to deal with it.

this was originally due to the API design - but by chatting in Slack we've managed to work around this 👍

radeksimko · 2017-06-27T12:28:20Z

azurerm/resource_arm_redis_cache.go

+							}, true),
+						},
+						"rdb_backup_frequency": {
+							Type:         schema.TypeString,


Similar question here - any reason we cannot make it TypeInt?

radeksimko · 2017-06-27T12:28:49Z

azurerm/resource_arm_redis_cache.go

+							ValidateFunc: validateRedisBackupFrequency,
+						},
+						"rdb_backup_max_snapshot_count": {
+							Type:     schema.TypeString,


☝️ TypeInt?

Updating the validation to use the Enum's

tombuildsstuff · 2017-06-28T13:22:09Z

Tests pass:

radeksimko

As mentioned via Slack this LGTM, as long as you can verify we don't need a state migration due to changes in the existing schema.

tombuildsstuff · 2017-06-28T14:49:51Z

I've confirmed there's no state migration needed by creating a Redis instance using a build from master, ensuring an empty plan - and then rebuilding with this branch + re-planning / updating values - LGTM 👍

Add Backup Support to `azurerm_redis_cache`

ghost · 2020-04-01T15:35:57Z

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. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

tombuildsstuff added the enhancement label Jun 26, 2017

radeksimko suggested changes Jun 27, 2017

View reviewed changes

tombuildsstuff added 9 commits June 28, 2017 09:50

Initial support for Redis Backups

f69a6b8

Adding in a state migration covering the redis backup setting

dca518d

Making the settings optional / adding tests

b0ac294

Updating the documentation to include the new settings

55c0c63

Need to use a Premium SKU to set the field at all

827d1f8

Removing the state migration

1f4b955

Adding a test updating the settings

5b6ca7d

Switching the backup params to be native types

9c41aec

Converting the existing fields to proper data types

61aed53

tombuildsstuff force-pushed the redis-backup-settings branch from d2f6626 to 61aed53 Compare June 28, 2017 10:28

tombuildsstuff added 6 commits June 28, 2017 12:41

Parsing values directly out of the schema if they're present

188f882

Updating the validation to use the Enum's

Converting the tests to use the functions

10ceac8

Updating the documentation to match

8573d08

Added a test for the backup frequency

91aa82b

Copy/Paste fail

5b3857e

Formatting

d0ee8e2

radeksimko approved these changes Jun 28, 2017

View reviewed changes

tombuildsstuff merged commit ab6ced3 into master Jun 28, 2017

tombuildsstuff deleted the redis-backup-settings branch June 28, 2017 15:11

tombuildsstuff added a commit that referenced this pull request Jun 28, 2017

Updating to include #130

f9b3dca

tombuildsstuff mentioned this pull request Jun 28, 2017

azurerm_storage_account - exposing a formatted Connection String for Blob access #142

Merged

tombuildsstuff added a commit that referenced this pull request Jul 4, 2017

Merge pull request #130 from terraform-providers/redis-backup-settings

66597bf

Add Backup Support to `azurerm_redis_cache`

tombuildsstuff added a commit that referenced this pull request Jul 4, 2017

Updating to include #130

fa1ae38

sebastus mentioned this pull request Jul 12, 2017

catch up with the team. (#1) #165

Closed

ghost locked and limited conversation to collaborators Apr 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Backup Support to `azurerm_redis_cache` #130

Add Backup Support to `azurerm_redis_cache` #130

tombuildsstuff commented Jun 26, 2017

tombuildsstuff commented Jun 26, 2017

tombuildsstuff commented Jun 26, 2017

radeksimko left a comment

radeksimko Jun 27, 2017 •

edited

Loading

tombuildsstuff Jun 28, 2017

radeksimko Jun 27, 2017

tombuildsstuff Jun 28, 2017

radeksimko Jun 27, 2017

tombuildsstuff Jun 28, 2017

tombuildsstuff commented Jun 28, 2017

radeksimko left a comment

tombuildsstuff commented Jun 28, 2017

ghost commented Apr 1, 2020

Add Backup Support to azurerm_redis_cache #130

Add Backup Support to azurerm_redis_cache #130

Conversation

tombuildsstuff commented Jun 26, 2017

tombuildsstuff commented Jun 26, 2017

tombuildsstuff commented Jun 26, 2017

radeksimko left a comment

Choose a reason for hiding this comment

radeksimko Jun 27, 2017 • edited Loading

Choose a reason for hiding this comment

tombuildsstuff Jun 28, 2017

Choose a reason for hiding this comment

radeksimko Jun 27, 2017

Choose a reason for hiding this comment

tombuildsstuff Jun 28, 2017

Choose a reason for hiding this comment

radeksimko Jun 27, 2017

Choose a reason for hiding this comment

tombuildsstuff Jun 28, 2017

Choose a reason for hiding this comment

tombuildsstuff commented Jun 28, 2017

radeksimko left a comment

Choose a reason for hiding this comment

tombuildsstuff commented Jun 28, 2017

ghost commented Apr 1, 2020

Add Backup Support to `azurerm_redis_cache` #130

Add Backup Support to `azurerm_redis_cache` #130

radeksimko Jun 27, 2017 •

edited

Loading