Site recovery support #4003

martenbohlin · 2019-08-03T20:09:47Z

This pull request adds 6 new resources needed to support site recovery (Azure to Azure).

This fixes issue #3480

Let me know if I can improve the code in any way.

…port site recovery.

…n order to support site recovery.

…ppings' in order to support site recovery.

…der to support site recovery.

martenbohlin · 2019-08-04T13:06:54Z

force-pushed to fix the lint errors that I had missed.

katbyte

Hi @martenbohlin,

Thank you for all these new resources! I've given this a first pass and its off to a great start. In addition to the comments i've left inline could we get some docs added for all the resources? As well as most of the errors need a little more detail added so when they happen we know what is erroring where.

thanks!

azurerm/resource_arm_recovery_services_fabric.go

katbyte · 2019-08-04T13:16:37Z

azurerm/resource_arm_recovery_services_fabric.go

+
+	d.Set("name", resp.Name)
+	d.Set("resource_group_name", resGroup)
+	d.Set("location", resp.Properties.FriendlyName) // Crazy? yes. But the location comes back in the friendly name


wow lol

There wil need to be a nil check here:

Suggested change

d.Set("location", resp.Properties.FriendlyName) // Crazy? yes. But the location comes back in the friendly name

if props := resp.Properties; props != nil {

d.Set("location", props.FriendlyName) // Crazy? yes. But the location comes back in the friendly name

}

azurerm/resource_arm_recovery_services_network_mapping.go

katbyte · 2019-08-04T13:18:14Z

azurerm/resource_arm_recovery_services_network_mapping.go

+				ForceNew:     true,
+				ValidateFunc: validate.NoEmptyStrings,
+			},
+			"name": {


Could we put this at the tope before any other properties for consistency with the rest of the provider?

katbyte · 2019-08-04T13:18:34Z

azurerm/resource_arm_recovery_services_network_mapping.go

+	return &schema.Resource{
+		Create: resourceArmRecoveryNetworkMappingCreate,
+		Read:   resourceArmRecoveryNetworkMappingRead,
+		Update: nil,


This can just be left blank

Suggested change

Update: nil,

katbyte · 2019-08-04T15:07:53Z

azurerm/resource_arm_recovery_services_protection_container.go

+				ForceNew:     true,
+				ValidateFunc: validate.NoEmptyStrings,
+			},
+			"name": {


Could we put this property up top as the first?

katbyte · 2019-08-04T15:12:56Z

azurerm/resource_arm_recovery_services_fabric.go

+		existing, err := client.Get(ctx, name)
+		if err != nil {
+			if !utils.ResponseWasNotFound(existing.Response) {
+				return fmt.Errorf("Error checking for presence of existing recovery services fabric: %+v", err)


For this (and all other error messages) could e be a little more clear about what is erroring:

Suggested change

return fmt.Errorf("Error checking for presence of existing recovery services fabric: %+v", err)

return fmt.Errorf("Error checking for presence of existing recovery services fabric %s (vault %s): %+v", name, vaultName, err)

katbyte · 2019-08-04T15:13:44Z

azurerm/resource_arm_recovery_services_protection_container.go

+
+	future, err := client.Create(ctx, fabricName, name, parameters)
+	if err != nil {
+		return fmt.Errorf("Error creating recovery services protection container1: %+v", err)


As with all errors could we add a bit more detail:

Suggested change

return fmt.Errorf("Error creating recovery services protection container1: %+v", err)

return fmt.Errorf("Error creating recovery services protection container %s (fabric %s): %+v", name, fabricName, err)

katbyte · 2019-08-04T15:14:05Z

azurerm/resource_arm_recovery_services_protection_container_mapping.go

+				Type:         schema.TypeString,
+				Required:     true,
+				ForceNew:     true,
+				ValidateFunc: validate.NoEmptyStrings,


COuld we validate this is an ID?

katbyte · 2019-08-04T15:14:13Z

azurerm/resource_arm_recovery_services_protection_container_mapping.go

+				Type:         schema.TypeString,
+				Required:     true,
+				ForceNew:     true,
+				ValidateFunc: validate.NoEmptyStrings,


COuld we validate this is an ID?

…orp#4003

martenbohlin · 2019-08-04T21:23:48Z

Thanks @katbyte for the quick feedback. I have fixed all your comments about the code, and I will start to look at the documentation now.

It would be great if you could double check commit 63dcc2e where I upgrade the azure-sdk-for-go dependency, I am not sure if I updated the files under vendor correctly.

…d redundant parameter source_network_name.

…r_mapping.

martenbohlin · 2019-08-05T16:54:56Z

All requested changes has now been made.

katbyte

Thanks for the revisions @martenbohlin,

I've left a couple more comments inline. WRT the sdk upgrade, we do that as a separate PR, i can open and get that merged for you once 1.33 is out.

I am seeing a few test failures thou:


------- Stdout: -------
=== RUN   TestAccAzureRMRecoveryReplicationPolicy_basic
=== PAUSE TestAccAzureRMRecoveryReplicationPolicy_basic
=== CONT  TestAccAzureRMRecoveryReplicationPolicy_basic

------- Stderr: -------
panic: Invalid address to set: []string{"location"}

looks like you are setting location, despite it not being part of the schema.

katbyte · 2019-08-13T19:26:07Z

azurerm/resource_arm_recovery_services_fabric.go

+	d.Set("name", resp.Name)
+	d.Set("resource_group_name", resGroup)
+	if props := resp.Properties; props != nil {
+		d.Set("location", props.FriendlyName) // Crazy? yes. But the location comes back in the friendly name


It might make sense to open an azure sdk bug for this one? 🤔

I found the correct way to get the location now that I have worked a bit more with the API.

katbyte · 2019-08-13T19:27:16Z

azurerm/resource_arm_recovery_services_network_mapping.go

+				Required:         true,
+				ForceNew:         true,
+				ValidateFunc:     validate.NoEmptyStrings,
+				DiffSuppressFunc: suppress.CaseDifference,


Now that we are fixing this at the d.Set(id) level can we remove this?

katbyte · 2019-08-13T19:27:20Z

azurerm/resource_arm_recovery_services_network_mapping.go

+				Required:         true,
+				ForceNew:         true,
+				ValidateFunc:     validate.NoEmptyStrings,
+				DiffSuppressFunc: suppress.CaseDifference,


azurerm/resource_arm_recovery_services_replicated_vm.go

katbyte · 2019-08-13T19:32:39Z

go.mod

@@ -1,7 +1,7 @@
 module github.com/terraform-providers/terraform-provider-azurerm

 require (
-	github.com/Azure/azure-sdk-for-go v31.0.0+incompatible
+	github.com/Azure/azure-sdk-for-go v31.1.0+incompatible


If we need to update the SDK that should be done in a separate PR, i can open that and get it merged for you once 1.33 goes out 🙂

Thanks, that sounds great!

Co-Authored-By: kt <kt@katbyte.me>

…corp#3480)

martenbohlin · 2019-08-14T13:07:11Z

About the test failures, I can not reproduce that. I ran make testacc TESTARGS='-run=TestAccAzureRMRecoveryReplicationPolicy' and get a success. And I have also tried to run the other tests that I have added.

katbyte

@martenbohlin,

The error is because you are setting a property that doesn't exist in schema. i've marked them with some comments. This behaviour is not yet enabled by default, but you can with the environment variable: TF_SCHEMA_PANIC_ON_ERROR=1

katbyte · 2019-08-18T14:46:54Z

azurerm/resource_arm_recovery_services_protection_container.go

+	}
+
+	d.Set("name", resp.Name)
+	d.Set("location", resp.Location)


You are setting location here and it doesn't exist in the schema

Aha, now I get it. Fixed.

katbyte · 2019-08-18T14:49:04Z

azurerm/resource_arm_recovery_services_replication_policy.go

+	d.Set("name", resp.Name)
+	d.Set("resource_group_name", resGroup)
+	d.Set("recovery_vault_name", vaultName)
+	d.Set("location", resp.Location)


katbyte

Thanks for the fix @martenbohlin! LGTM now 🚀

sean-nixon · 2019-08-19T22:57:01Z

Super appreciative of the work on this and excited to try this out. Sorry to chime in out of nowhere but are these typos intentional? targert_disk_type and targert_replica_disk_type. I noticed them in the documentation updates as well as in the source code from the replicated VM resource. Just wanted to call that out before this gets released

martenbohlin · 2019-08-20T11:18:59Z

@sean-nixon thanks for pointing that out. I have created pull request #4123 to fix the spelling mistakes.
@katbyte Will you merge that as well?

ghost · 2019-08-22T10:54:00Z

This has been released in version 1.33.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 1.33.0"
}
# ... other configuration ...

ghost · 2019-09-19T14:50:28Z

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!

ghost added size/XXL dependencies labels Aug 3, 2019

katbyte added this to the v1.34.0 milestone Aug 3, 2019

katbyte added service/recovery-services new-resource labels Aug 3, 2019

martenbohlin added 6 commits August 4, 2019 13:22

vendor: upgrading to v31.1.0 of github.com/Azure/azure-sdk-for-go

63dcc2e

Added support for creating 'Recovery Services Fabric' in order to sup…

185e5d0

…port site recovery.

Added support for creating 'Recovery Services Protection Container' i…

b66f338

…n order to support site recovery.

Added support for managing 'Recovery Services Replication Policies' i…

962bb78

…n order to support site recovery.

Added support for managing 'Recovery Services Protection Container Ma…

d91c2fe

…ppings' in order to support site recovery.

Added support for managing 'Recovery Services Network Mappings' in or…

063023b

…der to support site recovery.

martenbohlin force-pushed the site-recovery branch from 05bcd13 to 8f7c65b Compare August 4, 2019 11:26

Added support for replicating VMs in order to support site recovery.

c1a8e8a

martenbohlin force-pushed the site-recovery branch from 8f7c65b to c1a8e8a Compare August 4, 2019 12:33

katbyte requested changes Aug 4, 2019

View reviewed changes

katbyte added the waiting-response label Aug 4, 2019

Site recovery: Fixed katbytes initial comments in pull request hashic…

f43c608

…orp#4003

ghost removed the waiting-response label Aug 4, 2019

martenbohlin added 7 commits August 5, 2019 09:39

Merge remote-tracking branch 'origin/master' into site-recovery

636b0a1

Added documentation for azurerm_recovery_network_mapping. Also remove…

4a5510e

…d redundant parameter source_network_name.

Added documentation for azurerm_recovery_services_fabric

cf1647e

Added documentation for azurerm_recovery_services_protection_container.

3288910

Added documentation for azurerm_recovery_services_protection_containe…

6395902

…r_mapping.

Added dovcumentation for azurerm_recovery_services_replication_policy

3865fdc

Added documentation for azurerm_recovery_replicated_vm.

13230b5

martenbohlin added 2 commits August 12, 2019 11:30

Merge remote-tracking branch 'origin/master' into site-recovery

0f1a68a

Fixed failing integration tests. (hashicorp#4003)

de47d67

katbyte requested changes Aug 13, 2019

View reviewed changes

katbyte added the waiting-response label Aug 13, 2019

martenbohlin and others added 3 commits August 14, 2019 10:30

Removed unnecessary code. (hashicorp#3480)

4f6b9fc

Co-Authored-By: kt <kt@katbyte.me>

Found the correct way to get the location in the sdk-response. (hashi…

130c96b

…corp#3480)

Merge remote-tracking branch 'origin/master' into site-recovery

00ac03a

ghost removed the waiting-response label Aug 14, 2019

Merge remote-tracking branch 'origin/master' into site-recovery

8f383a9

katbyte requested changes Aug 18, 2019

View reviewed changes

katbyte added the waiting-response label Aug 18, 2019

martenbohlin added 2 commits August 19, 2019 13:30

Do not update state with property not in schema. (hashicorp#3480)

71fd508

Merge remote-tracking branch 'origin/master' into site-recovery

50492e3

katbyte approved these changes Aug 19, 2019

View reviewed changes

katbyte modified the milestones: v1.34.0, v1.33.0 Aug 19, 2019

katbyte merged commit 4b4cce5 into hashicorp:master Aug 19, 2019

katbyte added a commit that referenced this pull request Aug 19, 2019

Update CHANGELOG.md to include #4003

cf12210

ghost removed the waiting-response label Aug 19, 2019

ghost locked and limited conversation to collaborators Sep 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Site recovery support #4003

Site recovery support #4003

martenbohlin commented Aug 3, 2019

martenbohlin commented Aug 4, 2019

katbyte left a comment

katbyte Aug 4, 2019

katbyte Aug 4, 2019

katbyte Aug 4, 2019

martenbohlin Aug 14, 2019

katbyte Aug 4, 2019

martenbohlin Aug 14, 2019

katbyte Aug 4, 2019

katbyte Aug 4, 2019

katbyte Aug 4, 2019

katbyte Aug 4, 2019

martenbohlin commented Aug 4, 2019

martenbohlin commented Aug 5, 2019

katbyte left a comment

katbyte Aug 13, 2019

martenbohlin Aug 14, 2019

katbyte Aug 13, 2019

katbyte Aug 13, 2019

katbyte Aug 13, 2019

martenbohlin Aug 14, 2019

martenbohlin commented Aug 14, 2019

katbyte left a comment

katbyte Aug 18, 2019

martenbohlin Aug 19, 2019

katbyte Aug 18, 2019

katbyte left a comment

sean-nixon commented Aug 19, 2019

martenbohlin commented Aug 20, 2019

ghost commented Aug 22, 2019

ghost commented Sep 19, 2019

	return fmt.Errorf("Error checking for presence of existing recovery services fabric: %+v", err)
	return fmt.Errorf("Error checking for presence of existing recovery services fabric %s (vault %s): %+v", name, vaultName, err)

	return fmt.Errorf("Error creating recovery services protection container1: %+v", err)
	return fmt.Errorf("Error creating recovery services protection container %s (fabric %s): %+v", name, fabricName, err)

Site recovery support #4003

Site recovery support #4003

Conversation

martenbohlin commented Aug 3, 2019

martenbohlin commented Aug 4, 2019

katbyte 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

martenbohlin commented Aug 4, 2019

martenbohlin commented Aug 5, 2019

katbyte 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

martenbohlin commented Aug 14, 2019

katbyte 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

katbyte left a comment

Choose a reason for hiding this comment

sean-nixon commented Aug 19, 2019

martenbohlin commented Aug 20, 2019

ghost commented Aug 22, 2019

ghost commented Sep 19, 2019