azurerm_automation_schedule - Add missing properties for the advanced schedule #1626

TFaga · 2018-07-22T21:28:17Z

This pull request adds the missing properties in the azurerm_automation_schedule resource to configure the advanced schedule. Addresses issue #1392

…_schedule` resource

…e `azurerm_automation_schedule` resource

tombuildsstuff

hey @TFaga

Thanks for this PR :)

I've taken a look through and left some comments in-line but this is off to a good start; if we can fix up the comments then we should be able to run the tests and get this merged :)

Thanks!

tombuildsstuff · 2018-07-23T11:03:54Z

azurerm/resource_arm_automation_schedule.go

+
+			advancedSchedules, hasAdvancedSchedule := diff.GetOk("advanced_schedule")
+			if hasAdvancedSchedule {
+				if asl := advancedSchedules.([]interface{}); len(asl) > 0 && asl[0] != nil {


I think we can remove asl[0] != nil since that'll always be true if len(asl) > 0?

tombuildsstuff · 2018-07-23T11:07:14Z

azurerm/resource_arm_automation_schedule.go

+	//only pay attention to the advanced schedule if frequency is either Week or Month
+	if properties.Frequency == automation.Week || properties.Frequency == automation.Month {
+		if v, ok := d.GetOk("advanced_schedule"); ok {
+			if vl := v.([]interface{}); len(vl) > 0 && vl[0] != nil {


same here - I think we can remove vl[0] != nil since that's implied?

tombuildsstuff · 2018-07-23T11:08:34Z

azurerm/resource_arm_automation_schedule.go

@@ -257,6 +336,9 @@ func resourceArmAutomationScheduleRead(d *schema.ResourceData, meta interface{})
 	if v := resp.TimeZone; v != nil {
 		d.Set("timezone", v)
 	}
+	if v := resp.AdvancedSchedule; v != nil {
+		d.Set("advanced_schedule", flattenArmAutomationScheduleAdvanced(v))
+	}


can we ensure we set this all the time and move the nil-check within the flatten function? that ensures changes to this are detected. in addition - given this is a complex object can we check for errors when setting it? e.g.

if err := d.Set("advanced_schedule", flattenArmAutomationScheduleAdvanced(v)); err != nil { return fmt.Errorf("Error setting `advanced_schedule`: %+v", err) }

tombuildsstuff · 2018-07-23T11:10:02Z

azurerm/resource_arm_automation_schedule.go

+
+	expandedAdvancedSchedule := automation.AdvancedSchedule{}
+
+	if len(weekDays) > 0 {


can we remove this if statement - since it means it's not possible to remove these values once they've been set.

I've noticed an issue with this particular property. When creating the resource, if frequency is set to Month the week_days array cannot be set (even empty), otherwise the API returns an error. However during update it can and should be in order to reset it if it was removed from state. I've added a check for whether the current operation is a Create or Update and act accordingly. Perhaps separating the create and update methods would be a more clean solution?

tombuildsstuff · 2018-07-23T11:10:06Z

azurerm/resource_arm_automation_schedule.go

+		expandedAdvancedSchedule.WeekDays = &expandedWeekDays
+	}
+
+	if len(monthDays) > 0 {


can we remove this if statement - since it means it's not possible to remove these values once they've been set.

Same as with weekDays.

tombuildsstuff · 2018-07-23T11:14:12Z

azurerm/resource_arm_automation_schedule.go

+			flattenedMonthlyOccurrences[i] = f
+		}
+		result["monthly_occurrence"] = flattenedMonthlyOccurrences
+	}


tombuildsstuff · 2018-07-23T11:14:50Z

azurerm/resource_arm_automation_schedule.go

+	return &expandedAdvancedSchedule, nil
+}
+
+func flattenArmAutomationScheduleAdvanced(v *automation.AdvancedSchedule) []interface{} {


given we're now passing in a nil-value here, can we return an empty list e.g.

if v == nil { return []interface{}{} }

tombuildsstuff · 2018-07-23T11:16:32Z

azurerm/resource_arm_automation_schedule.go

+										Type:             schema.TypeString,
+										Required:         true,
+										DiffSuppressFunc: suppress.CaseDifference,
+										ValidateFunc:     validateScheduleDay(),


can we in-line this validation method for the moment? I have a feeling we may end up extracting out a listOfWeekDays method in time

tombuildsstuff · 2018-07-23T11:17:55Z

website/docs/r/automation_schedule.html.markdown

@@ -61,6 +65,22 @@ The following arguments are supported:

 * `timezone` - (Optional) The timezone of the start time. Defaults to `UTC`. For possible values see: https://msdn.microsoft.com/en-us/library/ms912391(v=winembedded.11).aspx

+* `advanced_schedule` - (Optional) Advanced fine-grained schedule frequency configuration. The `advanced_schedule` block supports fields documented below.
+
+The `advanced_schedule` block supports:


minor can we add --- above this line (and then a blank line) to add a divider

tombuildsstuff · 2018-07-23T11:18:15Z

website/docs/r/automation_schedule.html.markdown

+
+* `monthly_occurrence` - (Optional) List of occurrences of days within a month. The `monthly_occurrence` block supports fields documented below.
+
+The `monthly_occurrence` block supports:


(as above) can we add a divider here by adding --- and then a new line?

…roperties of the `azurerm_automation_schedule` resource

TFaga · 2018-07-23T13:23:05Z

Hey!

Thanks for the quick response and check. I've went through the comments and made the appropriate changes to the code.

I've however noticed one behavior that may be problematic. If you specify the frequency of the schedule as either Week or Month and don't add any advance scheduling properties, the API will still return a present (though empty) advancedSchedule object in the response, like so:

{
    "properties": {
        "interval": 1,
        "frequency": "Month",
        "advancedSchedule": {
            "monthDays": null,
            "monthlyOccurrences": null,
            "weekDays": null
        }
    }
}

This means that it gets saved in the state as having one item in the advanced_schedule while there are none specified in the .tf file. In conclusion this means that on each apply it will try to remove this item, which effectively won't do anything and making next apply do the same. Any thoughts on what best to do here?

Thanks!

…n_schedule` in case of state change to remove

tombuildsstuff · 2018-07-24T11:23:25Z

azurerm/resource_arm_automation_schedule.go

+	// If frequency is set to `Month` the `week_days` array cannot be set (even empty), otherwise the API returns an error.
+	// During update it can be set and it will not return an error. Workaround for the APIs behavior
+	if len(weekDays) > 0 || update {
+		expandedWeekDays := make([]string, len(weekDays))


so this second argument is the starting position in the array, if we make this 0 there's no empty element

tombuildsstuff · 2018-07-24T11:23:28Z

azurerm/resource_arm_automation_schedule.go

+
+	// Same as above with `week_days`
+	if len(monthDays) > 0 || update {
+		expandedMonthDays := make([]int32, len(monthDays))


so this second argument is the starting position in the array, if we make this 0 there's no empty element

tombuildsstuff · 2018-07-24T11:24:59Z

@TFaga hey, thanks for pushing those changes. Regarding the empty element that's present since the array creation (e.g. the make() method) takes the initial starting point of the array as the second element; since it's > 0 there's at least one blank element at the start of the array (which is where this is coming from). If we update this second argument to be 0 then this should work as expected :)

…te of the `azurerm_automation_schedule` resource

TFaga · 2018-07-24T14:32:21Z

@tombuildsstuff Hey, thanks for the quick response and info. I'm not sure I understand though, or rather I think I didn't phrase it very well. The problem that arises is that the schedule client/API is inconsistent with what properties of the advance schedule it deems valid on input.

When creating a new resource and for instance setting the frequency as Month the week_days property must not be set (not even array with length 0; it needs to be nil) or the call will return a validation error. Same deal for frequency Week and month_days property. However during an update it can be set to an array with length 0 and it will work as expected. That is why that if statement is there; during the initial creation of the resource, if the week_days or month_days are not set, the property for the client must not be set as well (they can and should be during an update).

To clarify with API examples.

This will error:

{
    "properties": {
        "frequency": "Month",
        "advancedSchedule": {
            "monthDays": [2],
            "monthlyOccurrences": [],
            "weekDays": [] // <- this
        }
    }
}

While this will succeed:

{
    "properties": {
        "frequency": "Month",
        "advancedSchedule": {
            "monthDays": [2],
            "monthlyOccurrences": [],
            "weekDays": null // <- this
        }
    }
}

Only on create, however. On update the first example will work without issue.

I've pushed a commit addressing the same problem if the whole advanced_schedule block is not present in the config or if it's removed from an existing resource and updated.

katbyte · 2018-07-27T18:58:46Z

Hi @TFaga,

You can always add a MinItems: 1 to weekDays property to prevent an empty array being passed in.

Additionally, might i suggest pulling monthDays, monthlyOccurrences and weekDays out of a block to the top level for simplicities sake? that would also help with the empty advancedSchedule response.

What do you think @tombuildsstuff ?

JunyiYi · 2018-08-08T00:29:01Z

website/docs/r/automation_schedule.html.markdown

+
+* `week_days` - (Optional) List of days of the week that the job should execute on.
+
+* `month_days` - (Optional) List of days of the month that the job should execute on. Must be between 1 and 31. 0 for last day of the month.


0 [](start = 112, length = 1)

minor can we quote the numbers in backtick such that " Must be between 1 and 31".
question are you sure 0 represents the last day of the month, or -1?

It's indeed -1. Thank you for the correction.

JunyiYi · 2018-08-08T00:31:06Z

website/docs/r/automation_schedule.html.markdown

+
+* `day` - (Required) Day of the occurrence. Must be one of Monday, Tuesday, Wednesday, Thursday, Friday, Saturday, Sunday.
+
+* `occurrence` - (Required) Occurrence of the week within the month. Must be between 1 and 5.


question tried with the Azure Rest API site, I also see -1 is available here (e.g. "Monday" -1 means the last Monday of a month). Can you confirm whether it is supported or not?

It's supported. Confirmed and added.

JunyiYi · 2018-08-08T00:35:38Z

azurerm/resource_arm_automation_schedule_test.go

@@ -338,3 +405,113 @@ func checkAccAzureRMAutomationSchedule_recurring_basic(resourceName string, freq
 		resource.TestCheckResourceAttr(resourceName, "timezone", "UTC"),
 	)
 }
+
+func testAccAzureRMAutomationSchedule_recurring_advanced_week(rInt int, location, frequency string, interval int, weekDay string) string {


interval int [](start = 100, length = 12)

suggestion The only possible value used in your test cases is 1, so you may hardcode the 1 in the HCL below and remove this parameter. Similar two suggestions below.

JunyiYi · 2018-08-08T00:40:42Z

azurerm/resource_arm_automation_schedule.go

@@ -107,14 +108,94 @@ func resourceArmAutomationSchedule() *schema.Resource {
 				//todo figure out how to validate this properly
 			},

-			//todo missing properties: week_days, month_days, month_week_day from advanced automation section
+			"advanced_schedule": {


"advanced_schedule" [](start = 3, length = 19)

suggestion We don't need to introduce wrapper advanced_schedule object here. What I mean is to move the mutual exclusive objects week_days, month_days and monthly_occurrence to the top level so we can reduce the depth of the hierarchy.

JunyiYi · 2018-08-08T00:43:26Z

azurerm/resource_arm_automation_schedule.go

+				MaxItems: 1,
+				Elem: &schema.Resource{
+					Schema: map[string]*schema.Schema{
+						"week_days": {


"week_days" [](start = 6, length = 11)

Since week_days, month_days and monthly_occurrence are mutually exclusive, why not declare ConflictsWith attribute in these three objects?

JunyiYi · 2018-08-08T00:44:28Z

azurerm/resource_arm_automation_schedule.go

+									"occurrence": {
+										Type:         schema.TypeInt,
+										Required:     true,
+										ValidateFunc: validation.IntBetween(1, 5),


validation.IntBetween(1, 5) [](start = 24, length = 27)

Can you confirm whether Go SDK supports -1 or not?

JunyiYi · 2018-08-08T00:46:29Z

azurerm/resource_arm_automation_schedule.go

-			if strings.ToLower(diff.Get("frequency").(string)) == "onetime" && interval.(int) > 0 {
-				return fmt.Errorf("interval canot be set when frequency is not OneTime")
+			if frequency == "onetime" && interval.(int) > 0 {
+				return fmt.Errorf("`interval` cannot be set when frequency is not OneTime")


"interval cannot be set when frequency is not OneTime" [](start = 22, length = 56)

interval cannot be set when frequency is ~~not~~ OneTime.

JunyiYi · 2018-08-08T01:02:12Z

Hi @TFaga , thanks for the contribution. I have looked through your code and provided some comments. Feel free to ask if some of them confused your.

…to reduce depth of the `azurerm_automation_schedule` resource

TFaga · 2018-08-09T12:22:38Z

Hi @JunyiYi . Thanks for the review. I've made the changes in the comments. I've moved the props one level up as was suggested. I agree it makes more sense, I wasn't sure whether to deviate from the SDK/API design.

katbyte

Hi @TFaga,

Thank you for the changes, most of my comments are minor improvements outside of the documentation. Look forward to getting this merged for you once these last few changes are made!

katbyte · 2018-08-09T15:59:01Z

azurerm/resource_arm_automation_schedule.go

+				Set:           set.HashStringIgnoreCase,
+				ConflictsWith: []string{"month_days", "monthly_occurrence"},
+			},
+			"month_days": {


Could we get an empty line here to make the spacing consistent?

katbyte · 2018-08-09T16:08:03Z

azurerm/resource_arm_automation_schedule.go

+
+func flattenArmAutomationScheduleAdvancedWeekDays(v *automation.AdvancedSchedule) *schema.Set {
+	flattenedWeekDays := schema.NewSet(set.HashStringIgnoreCase, []interface{}{})
+	if v.WeekDays != nil {


I believe this could be written as:

if weekDays := v.WeekDays; weekdays != nil { for i, v := range *weekDays { flattenedWeekDays.Add(v) } }

katbyte · 2018-08-09T16:08:49Z

azurerm/resource_arm_automation_schedule.go

+
+func flattenArmAutomationScheduleAdvancedMonthDays(v *automation.AdvancedSchedule) *schema.Set {
+	flattenedMonthDays := schema.NewSet(set.HashInt, []interface{}{})
+	if v.MonthDays != nil {


The same as above, could definitely be simplified.

katbyte · 2018-08-09T16:09:38Z

azurerm/resource_arm_automation_schedule.go

+func flattenArmAutomationScheduleAdvancedMonthlyOccurrences(v *automation.AdvancedSchedule) []map[string]interface{} {
+	flattenedMonthlyOccurrences := make([]map[string]interface{}, 0)
+	if v.MonthlyOccurrences != nil {
+		for i := range *v.MonthlyOccurrences {


Again, could use the syntax: for _, v := range monthlyOccurrences { and not have to deref and index so much.

katbyte · 2018-08-09T16:10:50Z

azurerm/resource_arm_automation_schedule_test.go

+%s
+
+resource "azurerm_automation_schedule" "test" {
+  name	 	              = "acctestAS-%d"


Could we make sure the =s are aligned here?

katbyte · 2018-08-09T16:11:03Z

azurerm/resource_arm_automation_schedule_test.go

+%s
+
+resource "azurerm_automation_schedule" "test" {
+  name	 	              = "acctestAS-%d"


Could we make sure the =s are aligned here?

katbyte · 2018-08-09T16:11:08Z

azurerm/resource_arm_automation_schedule_test.go

+%s
+
+resource "azurerm_automation_schedule" "test" {
+  name	 	              = "acctestAS-%d"


Could we make sure the =s are aligned here?

katbyte · 2018-08-09T16:14:18Z

website/docs/r/automation_schedule.html.markdown

@@ -61,6 +65,20 @@ The following arguments are supported:

 * `timezone` - (Optional) The timezone of the start time. Defaults to `UTC`. For possible values see: https://msdn.microsoft.com/en-us/library/ms912391(v=winembedded.11).aspx

+* `week_days` - (Optional) List of days of the week that the job should execute on. Only valid for `Week`.


Could we be a bit clearer by phrasing Only valid for 'Week' as Only valid when frequency is 'Week'? Applies to the new two properties to

katbyte · 2018-08-09T16:14:45Z

website/docs/r/automation_schedule.html.markdown

+
+The `monthly_occurrence` block supports:
+
+* `day` - (Required) Day of the occurrence. Must be one of Monday, Tuesday, Wednesday, Thursday, Friday, Saturday, Sunday.


Could we quote these values as such: Monday

… the `azurerm_automation_schedule` resource

TFaga · 2018-08-11T22:25:44Z

Hi @katbyte

Thanks for the quick review. I've made and pushed the changes in the comments.

katbyte

Hi @TFaga,

Thank you for the changes! this LGTM now 👍

katbyte · 2018-08-13T19:54:26Z

Tests pass:

ghost · 2020-03-30T15:04:16Z

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!

TFaga added 2 commits July 22, 2018 22:12

Added missing advanced schedule properties to the `azurerm_automation…

5e8bad9

…_schedule` resource

Added webside docs for the missing advanced schedule properties in th…

ab3ca15

…e `azurerm_automation_schedule` resource

tombuildsstuff added enhancement service/automation labels Jul 23, 2018

tombuildsstuff suggested changes Jul 23, 2018

View reviewed changes

tombuildsstuff added the waiting-response label Jul 23, 2018

Minor fixes to the code quality and diff behavior with the advanced p…

adab682

…roperties of the `azurerm_automation_schedule` resource

Updated handling of the advanced properties in the `azurerm_automatio…

22a9c1c

…n_schedule` in case of state change to remove

tombuildsstuff removed the waiting-response label Jul 24, 2018

tombuildsstuff reviewed Jul 24, 2018

View reviewed changes

tombuildsstuff added the waiting-response label Jul 24, 2018

Minor fixes to the handling of the advanced properties on create/upda…

980066b

…te of the `azurerm_automation_schedule` resource

metacpp requested a review from JunyiYi August 3, 2018 00:04

metacpp assigned JunyiYi Aug 3, 2018

JunyiYi reviewed Aug 8, 2018

View reviewed changes

Moved the advanced properties up one level with minor fixes in order …

7cdf594

…to reduce depth of the `azurerm_automation_schedule` resource

katbyte requested changes Aug 9, 2018

View reviewed changes

Minor optimization and formatting fixes of the advanced properties of…

279678b

… the `azurerm_automation_schedule` resource

katbyte approved these changes Aug 13, 2018

View reviewed changes

katbyte added this to the 1.13.0 milestone Aug 13, 2018

katbyte added a commit that referenced this pull request Aug 13, 2018

Update CHANGELOG.md to include #1626

6c5809d

katbyte merged commit fb5c2de into hashicorp:master Aug 13, 2018

TFaga deleted the iss1392-automation-schedule-missing-props branch August 14, 2018 20:03

TFaga restored the iss1392-automation-schedule-missing-props branch August 14, 2018 20:03

katbyte mentioned this pull request Oct 25, 2018

azurerm_automation_schedule is missing properties: week_days, month_days, month_week_day #1392

Closed

ghost locked and limited conversation to collaborators Mar 30, 2020


		expandedAdvancedSchedule := automation.AdvancedSchedule{}

		if len(weekDays) > 0 {


		* `monthly_occurrence` - (Optional) List of occurrences of days within a month. The `monthly_occurrence` block supports fields documented below.

		The `monthly_occurrence` block supports:


		* `week_days` - (Optional) List of days of the week that the job should execute on.

		* `month_days` - (Optional) List of days of the month that the job should execute on. Must be between 1 and 31. 0 for last day of the month.


		* `day` - (Required) Day of the occurrence. Must be one of Monday, Tuesday, Wednesday, Thursday, Friday, Saturday, Sunday.

		* `occurrence` - (Required) Occurrence of the week within the month. Must be between 1 and 5.

		@@ -61,6 +65,20 @@ The following arguments are supported:

		* `timezone` - (Optional) The timezone of the start time. Defaults to `UTC`. For possible values see: https://msdn.microsoft.com/en-us/library/ms912391(v=winembedded.11).aspx

		* `week_days` - (Optional) List of days of the week that the job should execute on. Only valid for `Week`.

azurerm_automation_schedule - Add missing properties for the advanced schedule #1626

azurerm_automation_schedule - Add missing properties for the advanced schedule #1626

Conversation

TFaga commented Jul 22, 2018

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

TFaga commented Jul 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombuildsstuff commented Jul 24, 2018

TFaga commented Jul 24, 2018

katbyte commented Jul 27, 2018

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

JunyiYi commented Aug 8, 2018

TFaga commented Aug 9, 2018

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

TFaga commented Aug 11, 2018

katbyte left a comment

Choose a reason for hiding this comment

katbyte commented Aug 13, 2018

ghost commented Mar 30, 2020

TFaga commented Jul 23, 2018 •

edited

Loading