Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

azurerm_automation_schedule - Add missing properties for the advanced schedule #1626

Merged
merged 7 commits into from
Aug 13, 2018
Merged

azurerm_automation_schedule - Add missing properties for the advanced schedule #1626

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5e8bad9
Added missing advanced schedule properties to the `azurerm_automation…
TFaga Jul 19, 2018
ab3ca15
Added webside docs for the missing advanced schedule properties in th…
TFaga Jul 22, 2018
adab682
Minor fixes to the code quality and diff behavior with the advanced p…
TFaga Jul 23, 2018
22a9c1c
Updated handling of the advanced properties in the `azurerm_automatio…
TFaga Jul 23, 2018
980066b
Minor fixes to the handling of the advanced properties on create/upda…
TFaga Jul 24, 2018
7cdf594
Moved the advanced properties up one level with minor fixes in order …
TFaga Aug 9, 2018
279678b
Minor optimization and formatting fixes of the advanced properties of…
TFaga Aug 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
175 changes: 172 additions & 3 deletions azurerm/resource_arm_automation_schedule.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"github.com/Azure/go-autorest/autorest/date"
"github.com/hashicorp/terraform/helper/schema"
"github.com/hashicorp/terraform/helper/validation"
"github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/set"
"github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/suppress"
"github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/validate"
"github.com/terraform-providers/terraform-provider-azurerm/azurerm/utils"
Expand Down Expand Up @@ -107,14 +108,78 @@ 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": {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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.

Type: schema.TypeList,
Optional: true,
MaxItems: 1,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"week_days": {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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?

Type: schema.TypeSet,
Optional: true,
Elem: &schema.Schema{
Type: schema.TypeString,
ValidateFunc: validateScheduleDay(),
},
Set: set.HashStringIgnoreCase,
},
"month_days": {
Type: schema.TypeSet,
Optional: true,
Elem: &schema.Schema{
Type: schema.TypeInt,
ValidateFunc: validate.IntBetweenAndNot(-1, 31, 0),
},
Set: set.HashInt,
},

"monthly_occurrence": {
Type: schema.TypeList,
Optional: true,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"day": {
Type: schema.TypeString,
Required: true,
DiffSuppressFunc: suppress.CaseDifference,
ValidateFunc: validateScheduleDay(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

},
},
},
},
},
},
},
},

CustomizeDiff: func(diff *schema.ResourceDiff, v interface{}) error {

frequency := strings.ToLower(diff.Get("frequency").(string))
interval, _ := diff.GetOk("interval")
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")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

interval cannot be set when frequency is not OneTime.

}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if frequency != "week" && frequency != "month" {
return fmt.Errorf("`advanced_schedule` can only be set when frequency is `Week` or `Month`")
}

as := asl[0].(map[string]interface{})
if frequency == "week" && as["week_days"].(*schema.Set).Len() == 0 {
return fmt.Errorf("`week_days` must be set when frequency is `Week`")
}
if frequency == "month" && as["month_days"].(*schema.Set).Len() == 0 && len(as["monthly_occurrence"].([]interface{})) == 0 {
return fmt.Errorf("Either `month_days` or `monthly_occurrence` must be set when frequency is `Month`")
}
}
}

_, hasAccount := diff.GetOk("automation_account_name")
Expand Down Expand Up @@ -193,6 +258,20 @@ func resourceArmAutomationScheduleCreateUpdate(d *schema.ResourceData, meta inte
properties.Interval = 1
}
}

//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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

advancedRef, err := expandArmAutomationScheduleAdvanced(vl)
if err != nil {
return err
}
properties.AdvancedSchedule = advancedRef
}
}
}

_, err := client.CreateOrUpdate(ctx, resGroup, accountName, name, parameters)
if err != nil {
return err
Expand Down Expand Up @@ -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))
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
}

return nil
}

Expand All @@ -282,3 +364,90 @@ func resourceArmAutomationScheduleDelete(d *schema.ResourceData, meta interface{

return nil
}

func expandArmAutomationScheduleAdvanced(v []interface{}) (*automation.AdvancedSchedule, error) {

// Get the one and only advance schedule configuration
advancedSchedule := v[0].(map[string]interface{})
weekDays := advancedSchedule["week_days"].(*schema.Set).List()
monthDays := advancedSchedule["month_days"].(*schema.Set).List()
monthlyOccurrences := advancedSchedule["monthly_occurrence"].([]interface{})

expandedAdvancedSchedule := automation.AdvancedSchedule{}

if len(weekDays) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

expandedWeekDays := make([]string, len(weekDays))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

for i := range weekDays {
expandedWeekDays[i] = weekDays[i].(string)
}
expandedAdvancedSchedule.WeekDays = &expandedWeekDays
}

if len(monthDays) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as with weekDays.

expandedMonthDays := make([]int32, len(monthDays))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

for i := range monthDays {
expandedMonthDays[i] = int32(monthDays[i].(int))
}
expandedAdvancedSchedule.MonthDays = &expandedMonthDays
}

if len(monthlyOccurrences) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem above is not present with monthlyOccurrences so I removed any checks here as advised.

expandedMonthlyOccurrences := make([]automation.AdvancedScheduleMonthlyOccurrence, len(monthlyOccurrences))
for i := range monthlyOccurrences {
m := monthlyOccurrences[i].(map[string]interface{})
occurrence := int32(m["occurrence"].(int))

expandedMonthlyOccurrences[i] = automation.AdvancedScheduleMonthlyOccurrence{
Occurrence: &occurrence,
Day: automation.ScheduleDay(m["day"].(string)),
}
}
expandedAdvancedSchedule.MonthlyOccurrences = &expandedMonthlyOccurrences
}

return &expandedAdvancedSchedule, nil
}

func flattenArmAutomationScheduleAdvanced(v *automation.AdvancedSchedule) []interface{} {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

result := make(map[string]interface{})

if v.WeekDays != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this could be written as:

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

flattenedWeekDays := schema.NewSet(set.HashStringIgnoreCase, []interface{}{})
for i := range *v.WeekDays {
flattenedWeekDays.Add((*v.WeekDays)[i])
}
result["week_days"] = flattenedWeekDays
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we update this to be:

flattenedWeekDays := schema.NewSet(set.HashStringIgnoreCase, []interface{}{})
if v.WeekDays != nil {
  for _, v := range *v.WeekDays {
    flattenedWeekDays.Add(v)
  }
}
result["week_days"] = flattenedWeekDays

this allows this to be set even if nil's returned from the API - which means diff's will be shown

if v.MonthDays != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same as above, could definitely be simplified.

flattenedMonthDays := schema.NewSet(set.HashInt, []interface{}{})
for i := range *v.MonthDays {
flattenedMonthDays.Add(int(((*v.MonthDays)[i])))
}
result["month_days"] = flattenedMonthDays
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(as above)

if v.MonthlyOccurrences != nil {
flattenedMonthlyOccurrences := make([]map[string]interface{}, len(*v.MonthlyOccurrences))
for i := range *v.MonthlyOccurrences {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

f := make(map[string]interface{})
f["day"] = (*v.MonthlyOccurrences)[i].Day
f["occurrence"] = int(*(*v.MonthlyOccurrences)[i].Occurrence)
flattenedMonthlyOccurrences[i] = f
}
result["monthly_occurrence"] = flattenedMonthlyOccurrences
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(as above)


return []interface{}{result}
}

func validateScheduleDay() schema.SchemaValidateFunc {

return validation.StringInSlice([]string{
string(automation.Monday),
string(automation.Tuesday),
string(automation.Wednesday),
string(automation.Thursday),
string(automation.Friday),
string(automation.Saturday),
string(automation.Sunday),
}, true)
}
Loading