Allow to specify daily time periods without chaos #62
Conversation
linki
changed the title
|
@twildeboer I overhauled your implementation of the |
| for _, tp := range strings.Split(timePeriods, ",") { | ||
| parts := strings.Split(tp, "-") | ||
| if len(parts) != 2 { | ||
| continue |
There was a problem hiding this comment.
Isn't this an error condition? Or are you just choosing to ignore it without logging it?
There was a problem hiding this comment.
Good point. I intended to silently ignore the case when there's only whitespace so that the default case "" doesn't need any special treatment. I'll provide a fix for when there's a value but doesn't seem to be valid time period, e.g. contains exactly one - I guess.
| continue | ||
| } | ||
|
|
||
| begin, err := time.ParseInLocation(timeFormatKitchen24, strings.TrimSpace(parts[0]), time.Local) |
There was a problem hiding this comment.
Don't you want to recognize the timezone set by the user in the config?
There was a problem hiding this comment.
Yeah, not sure what I did there. I think my intention was to just parse the string into an exactly corresponding time object regardless of any timezone, e.g. parsing a full time string ... 14:00 someTimezone into an object with hour=14. I think time.Parse without Location will just do that?!
There was a problem hiding this comment.
Seems like this should work. Just be careful and thorough in your testing. Maybe use UTC just for consistency with your TimeOfDay function?
| midnight := NewTimePeriod( | ||
| time.Date(1869, 9, 23, 23, 00, 00, 00, time.UTC), | ||
| time.Date(1869, 9, 24, 01, 00, 00, 00, time.UTC), | ||
| ) |
There was a problem hiding this comment.
Please correct me if I'm wrong, but I believe this is not a valid test because your actual code in ParseTimePeriods does not account for the beginning and ending times spanning midnight, so it would never actually produce the pair of begin and end times you are generating here. For instance, you have no test that correctly handles the excluded-times-of-day you give in the README: 22:00-08:00, which spans midnight. If you want to support that you have to detect that the end time is "earlier" than the begin time and add 24 hours to it. But beware, this will break during Daylight Saving nights and it will be very difficult to do this correctly (besides defining what "correct" is in that case). For example, what if the user sets the time period to be 23:00-02:11. On a Daylight Saving starting night, 02:11 never occurs, because it skips 02:00-02:59, and on the Daylight Saving end night 02:00-02:59 occurs twice! This is why I avoided supporting suspension periods spanning midnight. :-)
There was a problem hiding this comment.
Right, I'll definitely double-check that but I think it worked (apart from the daylight savings issue).
The idea is to use time.Parse to convert strings to time in the same manner as go does; however we don't care about the actual day and strip it off later. I believe the test above works with any day other than the 24th as well.
So this will end up in a time range that looks like start: 23, end: 1 (on day zero). start being greater than end idicates that the range goes across midnight which triggers a different logic branch for whether a time is included or not in Includes(): https://github.com/linki/chaoskube/pull/62/files#diff-99f38c7c07bfcc03a528180716bafab5R37
There was a problem hiding this comment.
Regarding daylight savings: If time.Parse returns the hours that would be shown on the clock, e.g. on start it jumps from 01:59 to 03:00 and on end it jumps from 02:59 to 02:00, then I think this should also work. Of course there will be one real hour more or less chaos time, but I think that's fine.
There was a problem hiding this comment.
Ah! Missed that bit of logic in Includes(). 👍
There was a problem hiding this comment.
Regarding daylight savings: ...
Makes sense. I just think it's important that you satisfy yourself that you have adequately addressed the potential issue. 👍
allows to specify time periods per day that chaoskube is inactive, e.g.