New resource: Automation Account/Credential/Runbook/Schedule #257

Kemyke · 2017-08-18T08:03:05Z

This PR contains the minimal required resources to create and schedule an automated runbook. The automation account is the container of the credential, runbook, and schedule.
You can set the content URI for your runbook to create from. I couldn't find any way to create runbook from inline script.

Currently only OneTime schedule is creatable due to this bug in azure rest api spec (Azure/azure-rest-api-specs#1545). The Interval field cannot be set in the Go SDK so recurring schedule cannot be constructed.

The StartTime is a tricky field in the schedule because only future time is acceptable (at least 5 minutes from now). It is only control the first run of the job, the next runs will occure regarding of the interval. So i didn't use the StartTime directly in the terraform schema but i introduced a first_run object where you can set when to run first. This way you don't have to set an exact date but you can control in which hour/minute/day/... you want the job to run first. The script will calculate the first possible start time from these fields.

~~I will attach the documentation soon where i will show you examples. But first i want to show the code to review.~~ Pushed.

…form-provider-azurerm into automation_resource

tombuildsstuff

Hey @Kemyke

Thanks for opening this PR and keeping up to date with all the recent changes - apologies for the delay in taking a look at this! I've taken a look through and left some comments in-line but this is off to a good start :)

The main question I've got is around the first_run field - I'm wondering if it'd make more sense as a data source, seeing as this could be reused in the Autoscale Settings fields too?

Thanks!

tombuildsstuff · 2017-08-31T08:09:17Z

azurerm/import_arm_automation_account_test.go

+	"github.com/hashicorp/terraform/helper/resource"
+)
+
+func TestAccAzureRMAutomationAccount_importAccoutWithFreeSku(t *testing.T) {


minor can we make Accout -> Account?

tombuildsstuff · 2017-08-31T08:09:22Z

azurerm/import_arm_automation_account_test.go

+	})
+}
+
+func TestAccAzureRMAutomationAccount_importAccoutWithBasicSku(t *testing.T) {


minor can we make Accout -> Account?

tombuildsstuff · 2017-08-31T08:12:05Z

azurerm/resource_arm_automation_account.go

+				ForceNew: true,
+			},
+
+			"location": {


can we swap this out for "location": locationSchema()?

tombuildsstuff · 2017-08-31T08:17:50Z

azurerm/resource_arm_automation_account.go

+			},
+
+			"sku": {
+				Type:     schema.TypeSet,


for newer resources we're converting things overt using List's instead of Set's - could we update this to using a List? That involves updating the type here to TypeList, removing the Set here and then updating the Expand/Flatten to use a List, as in this resource

tombuildsstuff · 2017-08-31T08:18:30Z

azurerm/resource_arm_automation_account.go

+			Sku: &sku,
+		},
+
+		Name:     &name,


we should be able to remove the name field here, since it's specified in the CreateOrUpdate method below

tombuildsstuff · 2017-08-31T09:45:37Z

website/docs/r/automation_credential.html.markdown

+
+* `password` - (Required) The password of the Credential.
+
+* `description` -  (Optional) A description for this Credential.


minor could we phrase this as The username associated with this Automation Credential to match the style of the other resources?

Done. You mean The description associated... i guess :)

Yes, copy and paste fail 🤦‍♂️

tombuildsstuff · 2017-08-31T09:46:38Z

website/docs/r/automation_runbook.html.markdown

+
+* `account_name` - (Required) The name of the automation account in which the Runbook is created. Changing this forces a new resource to be created.
+
+* `runbook_type` - (Required) The type of the runbook - can be either`Graph`, `GraphPowerShell`, `GraphPowerShellWorkflow`, `PowerShellWorkflow`, `PowerShell` or `Script`.


minor can we add a space between either and "Graph" here?

tombuildsstuff · 2017-08-31T10:00:07Z

azurerm/resource_arm_automation_schedule.go

+			return fmt.Errorf("Cannot parse start_time: %s", cst)
+		}
+	} else {
+		starttime = computeValidStartTime(d.Get("first_run").(*schema.Set), freq)


given this is going to be needed for a few resources - would this make more sense as a datasource? I have a feeling that in it's current form this will cause a perpetual diff?

tombuildsstuff · 2017-08-31T10:01:28Z

azurerm/resource_arm_automation_schedule.go

+	//interval :=
+
+	description := d.Get("description").(string)
+	timezone := "UTC"


can we make this configurable by the user, defaulted to UTC?

Done, docomentation is also updated

tombuildsstuff · 2017-08-31T10:01:51Z

website/docs/r/automation_schedule.html.markdown

+
+* `frequency` - (Required) The frequency of the schedule. - can be either `OneTime`, `Day`, `Hour`, `Week`, or `Month`.
+
+* `first_run` - (Optional) If an exact start time is not suitable, it can be used to make constraints for the first run. The start time will be calculated depending on these constraints. `start_time` will override this settings if defined.


(as above) would this make more sense as a data source? 🤔

Kemyke · 2017-08-31T12:02:35Z

Hi @tombuildsstuff

first of all, thank you for your review. I fixed the easy ones and pushed them. Now i need some time for the other comments to grasp them and i will do the modification ASAP. I will come back to you if i have questions.

Thank you!

Remove name AccountCreateOrUpdateProperties

tombuildsstuff

Hey @Kemyke

Thanks for pushing the latest updates - I've taken a look through and left a few comments but this is looking good :)

In addition to the comments we need to add the Resource Provider Registration to be able to provision Automation resources (for users who've not already registered them by hand / through the portal). We do this automatically when Terraform runs in this file, each Resource Provider is case sensitive, but it should just be a case of adding Microsoft.Automation to that list (which I've pulled from the Portal). Without the Resource Provider registered - users will see the error:

* azurerm_automation_account.example: automation.AccountClient#CreateOrUpdate: Failure responding to request: StatusCode=409 -- Original Error: autorest/azure: Service returned an error. Status=409 Code="MissingSubscriptionRegistration" Message="The subscription is not registered to use namespace 'Microsoft.Automation'. See https://aka.ms/rps-not-found for how to register subscriptions."

Otherwise, if we can fix the few comments I've noticed - we should be in a position to run the tests & merge this :)

Thanks!

tombuildsstuff · 2017-09-01T19:00:50Z

website/docs/r/automation_credential.html.markdown

+  account_name        = "${azurerm_automation_account.example.name}"
+  username           = "example_user"
+  password            = "example_pwd"
+  description         = "This is an example credential"


can we add in the missing } at the end of this example?

tombuildsstuff · 2017-09-01T19:03:39Z

azurerm/resource_arm_automation_credential.go

+	if props := resp.CredentialProperties; props != nil {
+		d.Set("username", props.UserName)
+	}
+	d.Set("password", nil)


if this isn't returned from the API - can we remove this line? this causes a perpetual diff in Terraform:

~ azurerm_automation_credential.example password: "<sensitive>" => "<sensitive>" (attribute changed)

tombuildsstuff · 2017-09-01T19:07:54Z

website/docs/r/automation_runbook.html.markdown

+resource "azurerm_automation_runbook" "example" {
+  name                = "Get-AzureVMTutorial"
+  location            = "${azurerm_resource_group.example.location}"
+  resource_group_name = "${azurerm_resource_group.exmaple.name}"


can we update exmaple -> example

tombuildsstuff · 2017-09-01T19:11:01Z

azurerm/resource_arm_automation_runbook.go

+		d.Set("description", props.Description)
+	}
+
+	d.Set("publish_content_link", nil) //publish content link is not set during Get()


from what I can see, this should be available in the response - is there a reason we're not setting this?

That was my first tought too but as you can see on the site you linked the publishContentLink is not listed in the example JSON. If i dump the result object of the Get it is also nil:

&automation.RunbookProperties{RunbookType:"PowerShellWorkflow", PublishContentLink:(*automation.ContentLink)(nil), State:"Published", LogVerbose:(*bool)(0xc420389168), LogProgress:(* bool)(0xc420389169), LogActivityTrace:(*int32)(0xc42038916c), JobCount:(*int32)(0xc4203891fc), Parameters:(*map[string]*automation.RunbookParameter)(0xc4202b83c0), OutputTypes:(*[]string)(0xc4204c41e0), Draft:(*automation.RunbookDraft)(nil), ProvisioningState:"Succeeded" , LastModifiedBy:(*string)(0xc420389260), CreationTime:(*date.Time)(0xc4204c4220), LastModifiedTime:(*date.Time)(0xc4204c4320), Description:(*string)(0xc4203891e0)}

In this case should i remove the nil association?

I removed, now its cleaner because it doesn't cause perpetual diff. As soon as it will be available in the response we can put it back.

tombuildsstuff · 2017-09-01T19:14:01Z

azurerm/resource_arm_automation_schedule.go

+			"start_time": {
+				Type:     schema.TypeString,
+				Required: true,
+			},


could we add some validation to check this is at least 5 minutes in the future? we can do that with a ValidateFunc in the Schema. Submitting a date in the past returns the following error which isn't particularly helpful:

* azurerm_automation_schedule.example: automation.ScheduleClient#CreateOrUpdate: Failure responding to request: StatusCode=400 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="Unknown" Message="Unknown service error"

tombuildsstuff · 2017-09-01T19:15:52Z

website/docs/r/automation_schedule.html.markdown

+  account_name        = "${azurerm_automation_account.example.name}"
+  frequency           = "OneTime"
+  timezone            = "Central Europe Standard Time"
+  start_time	      = "2014-04-15T18:00:15-07:00"


can we update the timezone on this to be +02:00 - otherwise users seeing this will get a perpetual diff:

~ azurerm_automation_schedule.example start_time: "2017-10-16T03:00:00+02:00" => "2017-10-15T18:00:15-07:00"

also using this example it appears that seconds aren't a valid interval here - do we need to validate for that?

~ azurerm_automation_schedule.example start_time: "2017-10-15T18:00:00+02:00" => "2017-10-15T18:00:15+02:00"

I think seconds are valid in the start time. You can set the start time on the Azure Website in second accuracy so i think 18:00:05 is a valid start time for a schedule.

Done. I only modified the timezone in the start_time.

Kemyke · 2017-09-06T12:25:41Z

Yeah, maybe you are right, azurerm_scheduled_time is maybe a good idea. In this case we can name the computed field as next_run_time.

What if we give an even more general name like azurerm_schedule or is it too general?

Kemyke · 2017-09-07T09:10:18Z

Hi @tombuildsstuff,

i have a next question regarding the azurerm_scheduled_time data source. If i am right i should use the GetOkExists function to retrieve for example the secondfield because the GetOk method returns ok = false if the value is 0. GetOkExists does what i actually need here. The problem is that this method was introduced in the beginning of august and it is not yet present in the azure provider source code. I think we should need to refresh the library under the vendor/github.com/hashicorp/terraform. Am i right? Is this something i can do? Is there any docs what are the steps to do this? Or can you explain how can i do it?

Regrdin this PR, could you take a look at the new code? Does it look good?

Thanks!

tombuildsstuff

Hey @Kemyke

Thanks for the continued effort here - apologies for the delay in re-reviewing this. I've taken another look through and have left some more comments - but this is looking good 😄

Thanks!

tombuildsstuff · 2017-09-06T11:37:57Z

azurerm/resource_arm_automation_account_test.go

+				Config: config,
+				Check: resource.ComposeTestCheckFunc(
+					testCheckAzureRMAutomationAccountExistsAndSku("azurerm_automation_account.test", automation.Basic),
+				),


can we update this to check the remote resource exists and then check the SKU in the state, rather than in Azure (since they should be the same, and the Import test should catch this not being set correctly in Azure):

resource.TestCheckResourceAttr(resourceName, "sku.0.name", "Basic"),

tombuildsstuff · 2017-09-06T11:38:22Z

azurerm/resource_arm_automation_account_test.go

+			{
+				Config: config,
+				Check: resource.ComposeTestCheckFunc(
+					testCheckAzureRMAutomationAccountExistsAndSku("azurerm_automation_account.test", automation.Free),


(same here) - can we check the field in the state, rather than in Azure (as the Import tests do that for us)

tombuildsstuff · 2017-09-06T11:40:17Z

azurerm/resource_arm_automation_account_test.go

+			return fmt.Errorf("Bad: Get on automationClient: %s", err)
+		}
+
+		if resp.Sku.Name != sku {


(as above) could we remove this check and fall back to checking the state, for the reasons outlined above?

tombuildsstuff · 2017-09-06T11:43:48Z

azurerm/resource_arm_automation_credential.go

+			},
+			"description": {
+				Type:     schema.TypeString,
+				Required: true,


from what I can see, this isn't Required and would be better as Optional: true & Computed: true?

It's a copy-paste bug i guess. Fixed!

tombuildsstuff · 2017-09-06T11:45:07Z

azurerm/resource_arm_automation_credential_test.go

+			return err
+		}
+
+		return fmt.Errorf("EventGrid Topic still exists:\n%#v", resp)


minor EventGrid Topic -> Automation Credential

tombuildsstuff · 2017-09-09T07:01:20Z

azurerm/resource_arm_automation_schedule.go

+	cst := d.Get("start_time").(string)
+	starttime, tperr := time.Parse(time.RFC3339, cst)
+	if tperr != nil {
+		return fmt.Errorf("Cannot parse start_time: %s", cst)


can we update this formatting argument to be %q to quote it?

tombuildsstuff · 2017-09-09T07:03:27Z

azurerm/resource_arm_automation_schedule.go

+	if v, ok := d.GetOk("timezone"); ok {
+		timezone = v.(string)
+	} else {
+		timezone = "UTC"


can we update the schema to set Default: "UTC"? That would allow us to just get this via d.Get()

tombuildsstuff · 2017-09-09T07:03:51Z

azurerm/resource_arm_automation_schedule.go

+			return nil
+		}
+
+		return fmt.Errorf("Error making Read request on AzureRM Automation Schedule '%s': %s", name, err)


minor can we make the secondary formatting argument %+v to return the full error here?

tombuildsstuff · 2017-09-09T07:04:34Z

azurerm/resource_arm_automation_schedule.go

+	d.Set("account_name", accName)
+	d.Set("frequency", resp.Frequency)
+	d.Set("description", resp.Description)
+	d.Set("start_time", string(resp.StartTime.Format(time.RFC3339)))


is there a reason we're not setting all the fields (such as timezone) here?

No, i just forgot to add that line. And the expiry time was also missing. I didn't even set the exipry time on the CreateOrUpdateProperties, i fixed that too.

tombuildsstuff · 2017-09-09T07:04:58Z

azurerm/resource_arm_automation_schedule_test.go

+			{
+				Config: config,
+				Check: resource.ComposeTestCheckFunc(
+					testCheckAzureRMAutomationScheduleExistsAndFrequencyType("azurerm_automation_schedule.test", automation.OneTime),


(as above) - can we update this to be testCheckAzureRMAutomationScheduleExists and check the values in the state?

…rnational/terraform-provider-azurerm into automation_resource

Kemyke · 2017-09-12T14:43:40Z

Hi @tombuildsstuff

can you please review the modifications? There is only one open quiestion, regarding the import_arm_automation_schedule_test.

Thanks!

tombuildsstuff · 2017-09-15T11:14:24Z

Hey @Kemyke

Sorry for the delay getting back to you here - I've replied in-line about that issue, but a custom DiffSuppressFunc should solve that for us :)

Thanks!

Kemyke · 2017-09-18T08:00:47Z

Hi @tombuildsstuff

i implemented a DiffSuppressFunc. Please check it if it is correct, if the old or new value couldn't be parsed, it returns a false. Is it the correct behavior?
And you were right, i have to check the start_time value without the second part because the Azure trimms it. And i also attach this DiffSuppressFunc for the expiry_time field.

Please check the new code!

Thanks for the review!

tombuildsstuff

Hey @Kemyke

Thanks for the continued effort on this PR - I've taken a look through and this LGTM :)

There's a couple of minor things which need doing to get this merged, but I'll take care of them now:

Vendoring the Azure SDK using govendor (rather than just coping the files in
Fixing the broken build (I caused this through a bad rebase, apologies)

I'll push updates for each of those shortly - but this otherwise LGTM :)

Thanks!

Also swapping the re-initialisation of `sender` to use the shared variable

tombuildsstuff · 2017-09-20T08:52:55Z

Tests pass:

Kemyke · 2017-09-20T10:03:18Z

Hi @tombuildsstuff ,

wow that's a really good news. I will create the next PR, with the azurerm_scheduled_time data source soon.

In the meanwhile, as a contributor in the Azure Go SDK, there was an issue there (Azure/azure-sdk-for-go#725) which implies a bug report in the azure rest api spec project but that bug is a little bit stucked (Azure/azure-rest-api-specs#1545), don't you have influence to move that issue on a bit? Because it is immposible to create a peroidic schedule without that fix.

Thanks!

ghost · 2020-04-01T15:19:30Z

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!

Kemyke added 4 commits August 17, 2017 13:09

Add automation resources: account, credential, runbook, schedule

d80fd0e

Add issue links

6c09115

Formatting

523e08e

Fix formatting and logging

1c34e27

Kemyke changed the title ~~Automation resources~~ New resource: Automation Account/Credential/Runbook/Schedule Aug 18, 2017

tombuildsstuff added enhancement new-resource labels Aug 18, 2017

Kemyke added 3 commits August 18, 2017 11:59

Add documentation

9bfd535

Merge branch 'master' into automation_resource

b969cbb

Merge branch 'master' into automation_resource

f607970

tombuildsstuff self-assigned this Aug 30, 2017

NEXOGEN\kemenya added 4 commits August 31, 2017 09:48

Merge branch 'master' of https://github.com/terraform-providers/terra…

c283cbf

…form-provider-azurerm into automation_resource

Fix auto-merge

3b04f54

Use new utils library

9002060

Fix method name

50fa597

tombuildsstuff reviewed Aug 31, 2017

View reviewed changes

root added 2 commits August 31, 2017 11:47

Minor fixes based on the review

b220c77

Fix formatting

72d53c6

root added 10 commits September 1, 2017 08:37

Refactor sku as List

a4d9c38

Remove name AccountCreateOrUpdateProperties

Get UserName from CredentialProperties instead of top level property

4d62cd9

Extend content_link schema with hash and version

cb9e4d9

Use properties from RunbookProperties

7dbda41

Use TypeList instead of TypeSet

fe3cd90

Add timezone to schedule

612a1a9

Remove unsupported interval

124f466

Remove interval from schema

3b9ee8d

Remove interval from docs

1b0425b

Remove first_run field and switch start_time as required field

fe7f536

tombuildsstuff reviewed Sep 1, 2017

View reviewed changes

tombuildsstuff reviewed Sep 9, 2017

View reviewed changes

root added 8 commits September 11, 2017 14:33

Fix test, add expirytime, fix logging

97b96a3

Merge branch 'automation_resource' of https://github.com/nexogen-inte…

df7f71a

…rnational/terraform-provider-azurerm into automation_resource

Remove Computed tag

6f5ea11

Check from state

7caaa74

Refactor credential tests

8d96426

Fix runbook import test

a540b75

Refactor logic

13a4bdb

Refactor schedule tests

0b5f90a

root added 3 commits September 18, 2017 07:29

Add diff function for start_time

816e856

Remove ExpectNonEmptyPlan

5cdc5d9

Add DiffSuppressFunc, formatting

2fe3b66

Merge branch 'master' into automation_resource

31d14da

tombuildsstuff approved these changes Sep 20, 2017

View reviewed changes

tombuildsstuff and others added 4 commits September 20, 2017 09:28

Fixing the broken build (caused by a bad rebase)

69b00d6

Also swapping the re-initialisation of `sender` to use the shared variable

Vendoring the Azure Automation SDK

231c7cc

Vendoring (plus comma)

d830bca

Upgrading to SDK 10.3

17a304c

tombuildsstuff merged commit 97bcbc5 into hashicorp:master Sep 20, 2017

tombuildsstuff added a commit that referenced this pull request Sep 20, 2017

Updating to include #257

6d083ca

Kemyke mentioned this pull request Sep 25, 2017

Add data_source_arm_scheduled_time data source and tests #353

Closed

ghost locked and limited conversation to collaborators Apr 1, 2020


		* `password` - (Required) The password of the Credential.

		* `description` - (Optional) A description for this Credential.


		* `account_name` - (Required) The name of the automation account in which the Runbook is created. Changing this forces a new resource to be created.

		* `runbook_type` - (Required) The type of the runbook - can be either`Graph`, `GraphPowerShell`, `GraphPowerShellWorkflow`, `PowerShellWorkflow`, `PowerShell` or `Script`.


		* `frequency` - (Required) The frequency of the schedule. - can be either `OneTime`, `Day`, `Hour`, `Week`, or `Month`.

		* `first_run` - (Optional) If an exact start time is not suitable, it can be used to make constraints for the first run. The start time will be calculated depending on these constraints. `start_time` will override this settings if defined.

New resource: Automation Account/Credential/Runbook/Schedule #257

New resource: Automation Account/Credential/Runbook/Schedule #257

Conversation

Kemyke commented Aug 18, 2017 • edited Loading

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

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

Kemyke commented Aug 31, 2017

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

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

Kemyke commented Sep 6, 2017

Kemyke commented Sep 7, 2017

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

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

Choose a reason for hiding this comment

Kemyke commented Sep 12, 2017

tombuildsstuff commented Sep 15, 2017

Kemyke commented Sep 18, 2017

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff commented Sep 20, 2017

Kemyke commented Sep 20, 2017 • edited Loading

ghost commented Apr 1, 2020

Kemyke commented Aug 18, 2017 •

edited

Loading

Kemyke commented Sep 20, 2017 •

edited

Loading