Schedule/feature/13054 rangespan#574
Conversation
Allow schedule ranges to span the midnight boundary properly. For instance, for a 9 PM to 3 AM change window you can simply state "range => '21:00 - 03:00'" instead of "range => ['21:00 - 23:59', '00:00 - 03:00' ]"
Part 2, adjust for the special adjustments needed to properly handle the day-of-week parameter when spanning days.
spec/unit/type/schedule_spec.rb
Outdated
There was a problem hiding this comment.
Based on the stubbed time and these ranges I don't see how this test is actually a different condition from the one before it.
There was a problem hiding this comment.
I see where you are coming from and it may be overkill. My thought process was that the condition before only tests non day-spanning times. These three tests actually test that when a range spanning days is given the behavior is essentially still the same. I could remove them if you'd like, but I was trying to be overly thorough with these tests.
There was a problem hiding this comment.
I feel like there's some confusion here. My comment refers to the range given on line 136 as opposed to the range on line 131, not the previous describe block. The tests say "when the start time is after the current time" and "when the end time is before the current time" but both of these conditions are true for both of those ranges. In fact it's necessary for both of those to be true in order for it not to match, since no day is specified, but having separate conditions in the test gives the impression that either condition is sufficient.
|
I find it a bit hard to read some of the tests (like the first one I commented on) because it's not obvious if phrases like "end time before the current time" is meant to take into account the day or if it's meant not to. If you were more explicit about all of the conditions in the test descriptions I think it would be helpful. |
|
Great feedback, thank you! I struggled with (in particular) the test descriptions. Let me go over these again taking your comments into account and I'll push up another commit once I've sorted through it. |
Enhance the descriptions provided for the units tests covering the new day-spanning range functionality.
|
I adjusted a few of the descriptions in this most recent commit. Hopefully that will clarify things a bit. Thanks! |
Incorporate feedback provided by Patrick Carlisle and fix two incorrect tests, remove one redundant unneeded test, and clarify the test description on another.
|
Patrick, I appreciate your time and attention on this (and patience with me for missing what you were saying the first time around). I have just pushed another commit which I believe addresses the issues you listed. Thanks! |
|
Reviewing this now. |
|
Thanks, this looks good now. I really like this change, thanks for spending the time on it. |
spec/unit/type/schedule_spec.rb
Outdated
There was a problem hiding this comment.
Can we include ScheduleTesting under the parent describe block to avoid doing it elsewhere?
There was a problem hiding this comment.
I don't see why not. I was just copying and pasting what was already present, but I've moved them all to the parent block and the tests still all pass so I have committed it and pushed it up.
Per Kelsey Hightower's suggestion, move the multiple 'include ScheduleTesting' statements in the specfile to the parent parent describe block.
|
@pcarlisle , @kelseyhightower : sounds like you guys have both looked over this one. Is it ready to go? I've only skimmed it, but I like the general idea and it passes specs and acceptance tests for me locally. I don't want it to get too stale--it's already got a merge conflict. We should rebase it and try to get it merged soon. |
|
Yes, it was good to go as far as I was concerned, just got sidetracked |
|
Chris, Is the rebasing something you'd like me to do or is that something you guys handle when you merge it? Sean |
|
We will be happy to do it when we get a chance, but if you were to do it before then, we certainly wouldn't be upset :) Thanks for the submission! |
|
Chris, I put a new pull request in at #642 since, while I'm not exactly a Git expert, from what I do know I believe pushing rebased commits up to an existing pull request is probably not a great idea. Thanks! Sean |
|
Rebased in #642 - closing this request. |
(PCP-736) Fix umask
4 participants
Please see feature request at https://projects.puppetlabs.com/issues/13054 for details and use cases.
Basically, allow ranges to span across midnight safely.
This is split into two commits, one to fix the basic range-spanning support across midnight, and then another that makes spanning across midnight work sensibly with the new weekday parameter recently merged (from https://projects.puppetlabs.com/issues/10328)
It comes with associated unit tests that I believe cover all of the relevant scenarios for this.
Thanks!