Codify implicit defaults for start and stop

[owenfarrell]

No description provided.

[vito]

I'm having a hard time reasoning about this default value - it looks like it's set to a "time of day" of 0, which is equivalent to 12:00AM (i.e. 0 seconds into the day). So having a "start" and "stop" of 0 implies that the only version that can possibly be returned is 12:00AM each day.

But then the resource just returns the current time. I could see this meaning "the start/stop has elapsed, so we should emit a timestamp," but that doesn't seem to be consistent with how the resource behaves when given start and stop and checking outside of the configured range:

❯ echo '{"source":{"start":"3AM","stop":"4AM"}}' | docker run -i concourse/time-resource /opt/resource/check
[]

[owenfarrell]

So having a "start" and "stop" of 0 implies that the only version that can possibly be returned is 12:00AM each day.

I see what you're saying, but I think that's a side effect of the fact that start is inclusive and stop is exclusive (according to the logic in TimeLord.Check). In other words, setting both values to 12:00AM means "on/after 12:00AM and before 12:00AM".

Without making other changes, a default start to 00:00 and a default stop to 23:59 will essentially exclude 23:59 from ever being generated as a version.

Using your scenario above: if you were to set both the start and stop to 3AM (or 4AM), check would always output the current timestamp... no matter the time of day.

[owenfarrell]

Using your scenario above: if you were to set both the start and stop to 3AM (or 4AM), check would always output the current timestamp... no matter the time of day.

One clarification.... this PR makes the above statement true by changing a time.Before condition to a !time.After condition.

[vito]

@owenfarrell I'm actually just confused as to what default behavior this intends to implement and how it would be used. Looking at the code, it configures a default start and stop of 0, which seems like it would make it unable to return anything. In my original comment I suggested that this would be because it would have to check at exactly 12AM, but now that you've pointed out that stop is exclusive, it's should be impossible to satisfy.

What does this configuration mean now?:

resources:
- name: ?
  type: time
  source: {}

I can't really think of any logical assumption for this, so having defaults feels odd. It looks like the tests suggest it should return the current timestamp, but that would leave the version history entirely subject to whenever checks are run, which doesn't seem "proper" - and I'm having a hard time squaring that test with my assumption that it'd be impossible to return anything at all.

Am I missing something? 🤔

[owenfarrell]

For the time being, I'm going to ignore the Source validation that's currently in place...

What does this configuration mean now?:

resources:
- name: ?
  type: time
  source: {}

As of v1.2.2, check will generate exactly 1 new version per day. And this PR doesn't change that behavior.

I can't really think of any logical assumption for this, so having defaults feels odd.

The part that I struggle with in the current implementation is that start and stop take on a totally different meaning based on whether or not an interval is defined.

• When an interval is defined, omitting start and stop will generate versions throughout the entire day.
• When no interval is defined, omitting start and stop will not generate any versions.

When I dug in to this, I came to the same realization that you did - you can't explicitly define a start/stop combination that represents a whole day. The only way to represent a whole day is implicitly, by omitting both start and stop.

This PR resolves both of those inconsistencies. So while these changes don't provide a ton of benefit to end users, these changes simplify the implementation of #53. A lot.

Rather than lump in a new constant with other, more complex enhancements, I thought it would be better to carve out these changes in to a separate PR to demonstrate that they are backwards compatible for all existing, valid configurations.

[vito]

Now that I've squinted my eyes and thought real hard, I think I can see that this PR is sort of 're-interpreting' the configuration in the following way:

• start defaults to the start of the current day.
• stop defaults to the start of the next day.
• without interval, the resource will generate one new version per start/stop range
• with interval, the resource will generate a new version on the given interval within the start/stop range

If that's the idea, it makes sense that we can remove the validation check, but I think this warrants a change to the README to document these defaults. I wrote the darn thing and it took this entire discussion for me to see it this way. 😅 start and stop were originally added after interval was introduced, and I just never thought of it in a way that could have default values.

[owenfarrell]

it makes sense that we can remove the validation check

My instinct was to leave some validation in place - if you define a start, you must define a stop, and vice versa. That way the default values would only kick in when both values are absent.

But it sounds like you're suggesting a complete removal of the (source Source) Validate() function. Am I hearing you right?

[vito]

But it sounds like you're suggesting a complete removal of the (source Source) Validate() function. Am I hearing you right?

Sorry, no, just what's already been removed by this PR.

Initially I wasn't a fan of its removal; an empty configuration was never meant to be valid, and any behavior that resulted from removing the validation was purely incidental. I wanted to make sure that by removing the validation we've made its behavior intentional and something that can be clearly documented in the README.

I was having a hard time figuring out what that intentional behavior would be, but I think I figured out what you're going for in #52 (comment) - is my interpretation accurate?

[owenfarrell]

Okay, cool.

I was having a hard time figuring out what that intentional behavior would be, but I think I figured out what you're going for in #52 (comment) - is my interpretation accurate?

Yeah, I think you're spot on. Your comments about how the resource behaves with and without the interval isn't anything new, but could use a callout.

I just pushed a commit to the README - the way I thought about start and stop is that they limit the creation of new time versions to the specified window. days created a similar precedent.

Aside - I tweaked the verbiage around days since the resource still runs 7 days a week, it just doesn't generate new versions. Semantics, I know.

[vito]

Much clearer now! Thanks for updating the rest of the README, it's a lot clearer. Just one small question/suggestion and we're good to go.

[vito]

👍 This is the detail I was missing that made everything make sense. It's a little strange to specify the same value for both, since the value itself doesn't matter, but it at least intuitively makes sense with the 12AM default.

Just for the sake of argument, it seems like it could be technically valid to specify start without stop and vice versa.

This would mean "every 10 minutes from 7PM to the end of the day":

interval: 10m
start: 7PM

And this would mean "every 10 minutes from the start of the day until 9AM":

interval: 10m
stop: 9AM

Is that right?

Not arguing for allowing it necessarily, just checking my logic.

In any case it might be worth mentioning that when not specified they both default to the same value, 12AM, and that that's how the resource achieves the "1 version per day" behavior when nothing is specified.

[owenfarrell]

Just for the sake of argument, it seems like it could be technically valid to specify start without stop and vice versa.

Yeah, that's why I got a bit confused by your comment about tossing the validation logic. You could throw out all the validation logic if you really want to.

This would mean "every 10 minutes from 7PM to the end of the day":
And this would mean "every 10 minutes from the start of the day until 9AM":
Is that right?

👍 Nailed it

Just pushed another README update to try to clarify the start/stop defaults. Hopefully that helps (and doesn't confuse things even more).

[vito]

Looks good to me - merging, thanks!