App Service: Add schedule backup functionality #3330

namku · 2019-04-29T12:28:01Z

Add schedule backup funcionality and documentation.

mbfrahry

Hey @namku, thanks for opening this PR. I can see you're on the right track but it needs a bit more work to get it merged - in particular:

There are some unintended consequences with how Terraform will behave and the approach you're using and I've left some comments explaining that below. I'd suggest wrapping everything into a backup List and rewrite what you did to incorporate that. It not only fixes up some issues Terraform will see but neatly keeps track of all the attributes needed for a backup.
It looks like the backup isn't being read into the statefile, you'll want to add that to the read function.
Once you get things rewritten, you'll need to add tests that create/update/delete a backup to make sure everything is hooked up and read into the statefile correctly.

Feel free to ask any questions if what I've said doesn't make sense.

mbfrahry · 2019-05-14T05:28:23Z

azurerm/helpers/azure/app_service_schedule_backup.go

+			Schema: map[string]*schema.Schema{
+				"frequency_interval": {
+					Type:     schema.TypeInt,
+					Required: true,


Is there any validation we can add here? Like greater than 0 and Less than some number?

azurerm/helpers/azure/app_service_schedule_backup.go

mbfrahry · 2019-05-14T05:33:49Z

azurerm/helpers/azure/app_service_schedule_backup.go

+				"retention_period_in_days": {
+					Type:     schema.TypeInt,
+					Optional: true,
+					Default:  30,


Thoughts around validation here too?

azurerm/resource_arm_app_service.go

mbfrahry · 2019-05-14T05:46:11Z

azurerm/resource_arm_app_service.go

@@ -269,6 +285,22 @@ func resourceArmAppServiceCreate(d *schema.ResourceData, meta interface{}) error
 		return err
 	}

+	backupSchedule := azure.ExpandAppServiceScheduleBackup(d.Get("backup_schedule"))


I don't believe we need this in the create method because this will be covered by the update that we go into immediately after creating.

mbfrahry · 2019-05-14T05:51:47Z

azurerm/resource_arm_app_service.go

@@ -338,6 +373,29 @@ func resourceArmAppServiceUpdate(d *schema.ResourceData, meta interface{}) error
 		}
 	}

+	if d.HasChange("backup_schedule") {


We're going to run into a perpetual Terraform state difference here if a user changes backup_name or storage_account_url without changing the backup_schedule since those wouldn't get updated unless backup_schedule has been changed. My above comment on wrapping everything into a backup TypeList should fix this

+1 to this - could we make this change?

Co-Authored-By: Matthew Frahry <[email protected]>

…azurerm

Co-Authored-By: Matthew Frahry <[email protected]>

…azurerm

tombuildsstuff

hey @namku

Thanks for pushing those changes - I've taken a look through and left some comments inline but this is looking fairly good to me. So that we can get this merged, I hope you don't mind but I'm going to push some commits to resolve the pending comments.

Since this PR was opened from the master branch of your fork, unfortunately that makes pushing a rebase complicated (since I can't push to that branch without breaking this PR). As such to do that I'm going to have to close this PR and open a new one, but this'll include the commits (and thus the credit) from this PR - I hope you don't mind; but this should allow us to ship this either way.

Thanks!

tombuildsstuff · 2019-07-07T18:45:54Z

azurerm/resource_arm_app_service.go

+				Type:         schema.TypeString,
+				Optional:     true,
+				ValidateFunc: validate.NoEmptyStrings,
+			},


could we move all of these into a backup block?

tombuildsstuff · 2019-07-07T18:46:09Z

azurerm/resource_arm_app_service.go

@@ -338,6 +373,29 @@ func resourceArmAppServiceUpdate(d *schema.ResourceData, meta interface{}) error
 		}
 	}

+	if d.HasChange("backup_schedule") {


+1 to this - could we make this change?

tombuildsstuff · 2019-07-07T18:46:40Z

azurerm/resource_arm_app_service.go

+	_, err = client.DeleteBackupConfiguration(ctx, resGroup, name)
+	if err != nil {
+		return err
+	}


rather than calling this method - could we switch to calling these 4 lined directly above?

tombuildsstuff · 2019-07-07T18:48:46Z

azurerm/resource_arm_app_service.go

+			}
+		}
+	}
+


could we also read these values back into the State in the Read function (the Read function contains some examples of this)? this allows diff's to be detected

tombuildsstuff · 2019-07-07T19:04:23Z

examples/app-service/scheduler-backup/README.md

@@ -0,0 +1,3 @@
+# Example: a Basic schedule backup App Service


could we make this:

Suggested change

# Example: a Basic schedule backup App Service

# Example: an App Service with a Scheduled Backup

tombuildsstuff · 2019-07-07T19:04:38Z

examples/app-service/scheduler-backup/README.md

@@ -0,0 +1,3 @@
+# Example: a Basic schedule backup App Service
+
+This example provisions a schedule backup App Service.


could we make this:

Suggested change

This example provisions a schedule backup App Service.

This example provisions an App Service with a Backup configured.

``` $ acctests azurerm TestAccAzureRMAppService_backup === RUN TestAccAzureRMAppService_backup === PAUSE TestAccAzureRMAppService_backup === CONT TestAccAzureRMAppService_backup --- PASS: TestAccAzureRMAppService_backup (660.26s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 660.313s ``` ``` $ acctests azurerm TestAccAzureRMAppService_basic === RUN TestAccAzureRMAppService_basic === PAUSE TestAccAzureRMAppService_basic === RUN TestAccAzureRMAppService_basicWindowsContainer === PAUSE TestAccAzureRMAppService_basicWindowsContainer === CONT TestAccAzureRMAppService_basic --- PASS: TestAccAzureRMAppService_basic (189.59s) === CONT TestAccAzureRMAppService_basicWindowsContainer --- PASS: TestAccAzureRMAppService_basicWindowsContainer (369.53s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 559.155s ```

tombuildsstuff · 2019-07-07T19:59:32Z

hey @namku

Apologies for the delayed re-review here!

So that we can get this merged I've pulled these commits and the changes requested above into #3804 (since unfortunately it's not possible to rebase the master branch of a fork without breaking the PR, due to a bug with Github's Branch Protection) - which I'm going to close this in favour of #3804 (which includes your commits so you'll still be attributed for this contribution) - I hope you don't mind!

Thanks!

``` $ acctests azurerm TestAccAzureRMAppService_backup === RUN TestAccAzureRMAppService_backup === PAUSE TestAccAzureRMAppService_backup === CONT TestAccAzureRMAppService_backup --- PASS: TestAccAzureRMAppService_backup (660.26s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 660.313s ``` ``` $ acctests azurerm TestAccAzureRMAppService_basic === RUN TestAccAzureRMAppService_basic === PAUSE TestAccAzureRMAppService_basic === RUN TestAccAzureRMAppService_basicWindowsContainer === PAUSE TestAccAzureRMAppService_basicWindowsContainer === CONT TestAccAzureRMAppService_basic --- PASS: TestAccAzureRMAppService_basic (189.59s) === CONT TestAccAzureRMAppService_basicWindowsContainer --- PASS: TestAccAzureRMAppService_basicWindowsContainer (369.53s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 559.155s ```

ghost · 2019-07-30T03:49:07Z

This has been released in version 1.32.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.32.0"
}
# ... other configuration ...

ghost · 2019-08-07T14:50:22Z

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 👉 [email protected]. Thanks!

namku added 2 commits April 29, 2019 14:14

Add schedule backup functionality.

96c45ec

Add doc in app_service.html.markdown file

26d6238

ghost added size/L documentation labels Apr 29, 2019

namku added 5 commits April 29, 2019 15:06

Add deafult value to backup_schedule_enabled

8cf19ab

Add default value to backup_schedule_enabled parameter

26aff5b

delete backup-schedule_enabled parameter

07d21f1

Delete backupScheduleEnabled param

58bc928

Add condition err in resourceArmDeleteScheduleBackup

3f3b4d0

katbyte added enhancement service/app-service labels Apr 29, 2019

namku added 3 commits April 29, 2019 19:14

In deleteConfiguration function delete check

35aaa54

Add HasChange in update function

cac2b23

Fix errcheck in DeleteBackupConfiguration

aad82ef

katbyte force-pushed the master branch from 77d8dd0 to fa53685 Compare May 12, 2019 19:43

mbfrahry requested changes May 14, 2019

View reviewed changes

tombuildsstuff added the waiting-response label May 14, 2019

namku added 2 commits May 25, 2019 10:16

Frequency interval validation range limit

4db2c17

Delete backupSchedule in create function

e106789

ghost added size/XL and removed size/L labels May 25, 2019

namku and others added 8 commits May 25, 2019 12:15

Update azurerm/resource_arm_app_service.go

4bb7dd3

Co-Authored-By: Matthew Frahry <[email protected]>

Validate retention period range limit

784c654

Merge branch 'master' of https://github.com/namku/terraform-provider-…

9d150ee

…azurerm

Update azurerm/resource_arm_app_service.go

27933c4

Co-Authored-By: Matthew Frahry <[email protected]>

Fix tabulation and add validate to backup_name

e40f9d9

Merge branch 'master' of https://github.com/namku/terraform-provider-…

dbbe58a

…azurerm

Add frequency_unit validation

7e5b03f

Merge branch 'backupSchedule_useless_variable'

64ceb64

ghost removed the size/XL label May 25, 2019

ghost added the size/L label May 25, 2019

tombuildsstuff self-assigned this Jul 7, 2019

tombuildsstuff added this to the v1.32.0 milestone Jul 7, 2019

tombuildsstuff approved these changes Jul 7, 2019

View reviewed changes

tombuildsstuff mentioned this pull request Jul 7, 2019

r/app_service: support for backups #3804

Merged

tombuildsstuff closed this Jul 7, 2019

ghost removed the waiting-response label Jul 30, 2019

ghost locked and limited conversation to collaborators Aug 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

App Service: Add schedule backup functionality #3330

App Service: Add schedule backup functionality #3330

namku commented Apr 29, 2019

mbfrahry left a comment

mbfrahry May 14, 2019

mbfrahry May 14, 2019

mbfrahry May 14, 2019

mbfrahry May 14, 2019

tombuildsstuff Jul 7, 2019

tombuildsstuff left a comment

tombuildsstuff Jul 7, 2019

tombuildsstuff Jul 7, 2019

tombuildsstuff Jul 7, 2019

tombuildsstuff Jul 7, 2019

tombuildsstuff Jul 7, 2019

tombuildsstuff Jul 7, 2019

tombuildsstuff commented Jul 7, 2019

ghost commented Jul 30, 2019

ghost commented Aug 7, 2019

		@@ -0,0 +1,3 @@
		# Example: a Basic schedule backup App Service

	# Example: a Basic schedule backup App Service
	# Example: an App Service with a Scheduled Backup

		@@ -0,0 +1,3 @@
		# Example: a Basic schedule backup App Service

		This example provisions a schedule backup App Service.

	This example provisions a schedule backup App Service.
	This example provisions an App Service with a Backup configured.

App Service: Add schedule backup functionality #3330

App Service: Add schedule backup functionality #3330

Conversation

namku commented Apr 29, 2019

mbfrahry 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

tombuildsstuff 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

tombuildsstuff commented Jul 7, 2019

ghost commented Jul 30, 2019

ghost commented Aug 7, 2019