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

Jump to bottom

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 5 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
194 changes: 191 additions & 3 deletions azurerm/resource_arm_automation_schedule.go
View file
Open in desktop
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,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": {
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: 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),
},
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: 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),
},
"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 {
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 +274,28 @@ 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 {
isUpdate := d.Id() != ""
if v, ok := d.GetOk("advanced_schedule"); ok {
if vl := v.([]interface{}); len(vl) > 0 {
advancedRef, err := expandArmAutomationScheduleAdvanced(vl, isUpdate)
if err != nil {
return err
}
properties.AdvancedSchedule = advancedRef
}
} else if isUpdate {
// To make sure all the properties are unset in the event of removal update, pass in an empty skeleton object
properties.AdvancedSchedule = &automation.AdvancedSchedule{
WeekDays: &[]string{},
MonthDays: &[]int32{},
MonthlyOccurrences: &[]automation.AdvancedScheduleMonthlyOccurrence{},
}
}
}

_, err := client.CreateOrUpdate(ctx, resGroup, accountName, name, parameters)
if err != nil {
return err
Expand Down Expand Up @@ -257,6 +360,9 @@ func resourceArmAutomationScheduleRead(d *schema.ResourceData, meta interface{})
if v := resp.TimeZone; v != nil {
d.Set("timezone", v)
}
if err := d.Set("advanced_schedule", flattenArmAutomationScheduleAdvanced(resp.AdvancedSchedule)); err != nil {
return fmt.Errorf("Error setting `advanced_schedule`: %+v", err)
}
Copy link
Member

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 +388,85 @@ func resourceArmAutomationScheduleDelete(d *schema.ResourceData, meta interface{

return nil
}

func expandArmAutomationScheduleAdvanced(v []interface{}, isUpdate bool) (*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 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 || isUpdate {
expandedWeekDays := make([]string, len(weekDays))
Copy link
Member

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
}

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

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
}

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
Member

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

// If all the elements in the advanced schedule are `nil` it's assumed it doesn't exist
if v == nil || (v.WeekDays == nil && v.MonthDays == nil && v.MonthlyOccurrences == nil) {
return []interface{}{}
}

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

flattenedWeekDays := schema.NewSet(set.HashStringIgnoreCase, []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)
		}
	}

for i := range *v.WeekDays {
flattenedWeekDays.Add((*v.WeekDays)[i])
}
}
Copy link
Member

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

result["week_days"] = flattenedWeekDays

flattenedMonthDays := schema.NewSet(set.HashInt, []interface{}{})
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.

for i := range *v.MonthDays {
flattenedMonthDays.Add(int(((*v.MonthDays)[i])))
}
}
Copy link
Member

Choose a reason for hiding this comment

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

(as above)

result["month_days"] = flattenedMonthDays

flattenedMonthlyOccurrences := make([]map[string]interface{}, 0)
if v.MonthlyOccurrences != nil {
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 = append(flattenedMonthlyOccurrences, f)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

(as above)

result["monthly_occurrence"] = flattenedMonthlyOccurrences

return []interface{}{result}
}