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: skip set location if expiry_time exceed the upper limit as year 9999 #21886

Merged
merged 3 commits into from
Jun 26, 2023
Merged

azurerm_automation_schedule: skip set location if expiry_time exceed the upper limit as year 9999 #21886

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fcf716c
skip set location if expiry_time exceed the upper limit as year 9999
wuxu92 May 23, 2023
afbb5bc
use raw string for schedule expiry time, and add a new acc test
wuxu92 Jun 15, 2023
90e6335
golangci import sort
wuxu92 Jun 20, 2023
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
10 changes: 7 additions & 3 deletions internal/services/automation/automation_schedule_resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ func resourceAutomationSchedule() *pluginsdk.Resource {
Type: pluginsdk.TypeString,
Optional: true,
Computed: true,
DiffSuppressFunc: suppress.RFC3339Time,
DiffSuppressFunc: suppress.RFC3339MinuteTime,
ValidateFunc: validation.IsRFC3339Time,
// defaults to now + 7 minutes in create function if not set
},
Expand All @@ -95,7 +95,7 @@ func resourceAutomationSchedule() *pluginsdk.Resource {
Type: pluginsdk.TypeString,
Optional: true,
Computed: true, // same as start time when OneTime, ridiculous value when recurring: "9999-12-31T15:59:00-08:00"
DiffSuppressFunc: suppress.CaseDifference,
DiffSuppressFunc: suppress.RFC3339MinuteTime,
ValidateFunc: validation.IsRFC3339Time,
},

Expand Down Expand Up @@ -263,7 +263,11 @@ func resourceAutomationScheduleCreateUpdate(d *pluginsdk.ResourceData, meta inte

if v, ok := d.GetOk("expiry_time"); ok {
t, _ := time.Parse(time.RFC3339, v.(string)) // should be validated by the schema
parameters.Properties.SetExpiryTimeAsTime(t.In(loc))
// fixes: https://github.com/hashicorp/terraform-provider-azurerm/issues/21854. that year 9999 may return by API
if t.In(loc).Year() <= 9999 {
t = t.In(loc)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason we can't always send and return UTC here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes I think it can always send a UTC time here. but I am not sure if it will cause changes to current users if we change to UTC here.

the timezone logic introduced in this PR if @catriona-m has any context about this logic and can we change the loc to UTC safely?

Copy link
Member

Choose a reason for hiding this comment

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

if we have a user-specified value here, we would still need it to be in the timezone they have specified too I think, so we do need to check if it's the default value from the api or not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tombuildsstuff Is this PR acceptable to resolve the issue, or do you prefer @catriona-m's suggestion to avoid setting expiry_time as a timestamp in the Read in first place if it hasn't been specified? both options seem viable to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

actually we don't need to do either approach, we can instead set the raw string value rather than parsing and then setting the date time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that should work too. I'll update it that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR updated and added a new acc test for it:

--- PASS: TestAccAutomationSchedule_oneTime_basic (158.45s)
--- PASS: TestAccAutomationSchedule_expiryTimeOfEuropeTimeZone (195.06s)
--- PASS: TestAccAutomationSchedule_requiresImport (166.47s)
--- PASS: TestAccAutomationSchedule_oneTime_complete (95.21s)
--- PASS: TestAccAutomationSchedule_oneTime_update (126.64s)
--- PASS: TestAccAutomationSchedule_hourly (91.86s)
--- PASS: TestAccAutomationSchedule_daily (94.86s)
--- PASS: TestAccAutomationSchedule_weekly (94.92s)
--- PASS: TestAccAutomationSchedule_monthly (154.84s)
--- PASS: TestAccAutomationSchedule_weekly_advanced (94.75s)
--- PASS: TestAccAutomationSchedule_monthly_advanced_by_day (92.64s)
--- PASS: TestAccAutomationSchedule_monthly_advanced_by_week_day (93.93s)

parameters.Properties.SetExpiryTimeAsTime(t)
}

// only pay attention to interval if frequency is not OneTime, and default it to 1 if not set
Expand Down