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.

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

fix(maintenance): timezone handling offset issue #981

Merged
merged 4 commits into from
Jan 29, 2025

Conversation

alexmaras
Copy link
Contributor

@alexmaras alexmaras commented Jan 28, 2025

Summary

Fixes an issue with the timezone handling in Maintenance blocks
#911

The problem was with using Truncate, which always operates in UTC. Switching to constructing the date from component parts (0, 0, 0, 0 for midnight) fixes this issue and makes the time always midnight for the timezone the maintenance window refers to.

I've added a test for this too - the previous test didn't catch the problem because it was only an hour out from UTC, and the window covered a 2 hour period. I've using UTC+8 (Australia/Perth) for a large enough gap to catch the issue. This test fails without the code changes, and passes now.

Checklist

  • Tested and/or added tests to validate that the changes work as intended, if applicable.
  • Updated documentation in README.md, if applicable. (Not applicable - bugfixing existing behavior)

@alexmaras
Copy link
Contributor Author

(just a heads-up - fairly new to golang, so let me know if there's any more go-typical ways of handling things)

Copy link
Owner

@TwiN TwiN left a comment

Choose a reason for hiding this comment

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

Excellent catch! Just a small nitpick with the newlines, but looks perfect otherwise!

Comment on lines 114 to 122

adjustedDate := now.Day()
if now.Hour() < int(c.durationToStartFromMidnight.Hours()) {
// if time in maintenance window is later than now, treat it as yesterday
adjustedDate--
}
// Set to midnight prior to adding duration
dayWhereMaintenancePeriodWouldStart = time.Date(now.Year(), now.Month(), adjustedDate, 0, 0, 0, 0, now.Location())

Copy link
Owner

Choose a reason for hiding this comment

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

Can you remove the blank lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👍

Repository owner deleted a comment from codecov bot Jan 29, 2025
@alexmaras
Copy link
Contributor Author

I also just realised I can remove the separate variable definition for dayWhereMaintenancePeriodWouldStart as it's not being set in the if/else now.

@alexmaras alexmaras requested a review from TwiN January 29, 2025 02:58
@TwiN TwiN changed the title Fix/timezone handling maintenance fix(maintenance): timezone handling offset issue Jan 29, 2025
Copy link

codecov bot commented Jan 29, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.86%. Comparing base (dd839be) to head (faf7640).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #981   +/-   ##
=======================================
  Coverage   75.86%   75.86%           
=======================================
  Files          74       74           
  Lines        6758     6758           
=======================================
  Hits         5127     5127           
  Misses       1325     1325           
  Partials      306      306           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@TwiN TwiN merged commit 975ac35 into TwiN:master Jan 29, 2025
2 checks passed
@TwiN
Copy link
Owner

TwiN commented Jan 29, 2025

@alexmaras Thank you for the contribution!

@TwiN TwiN added the bug Something isn't working label Jan 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants