-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.
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
Conversation
also suppress diff ignore second for start_time and expiry_time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for submitting this @wuxu92. While this may resolve the error in the linked issue, a different approach here may be more useful. Perhaps we could avoid setting expiry_time as a timestamp in the Read in first place if it hasn't been specified?
// 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) | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
@catriona-m the API will reponse expiry_time as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @wuxu92 LGTM 🌮
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
also update
DiffSuppressFunc
start_time and expiry_time. or it will cause diff because the API will remove the second part in response value for these two field. like belowfixes: #21854