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

Make time resource versions more predictable #56

Closed
wants to merge 9 commits into from

Conversation

owenfarrell
Copy link
Member

@owenfarrell owenfarrell commented Sep 3, 2020

Fixes #45

Take 2...

This PR integrates #53 and #54 in to the functions called by Concourse to provide predictable generation. As a result, this resource now strictly adheres to the contract for implementing a resource and that anyone who wants to trigger jobs for every version can configure the pipeline to do just that. So now is the time to pay off any scrutiny that was deferred as part of my previous PRs.

A couple of notes:

  • As mentioned in Extract business logic to base package #55, relying on time.Now() is pretty risky given the desire to offset new versions across a range/interval. As part of this PR, I defined a function (defaulting to time.Now()) which allows for a static override of the time when running unit tests. Since the unit tests for check adjust the start/stop configuration based on the perceived time, I don't think pinning the clock is an issue. If there are any scenarios you can think of that I've overlooked, please shout.
  • One side effect of this PR is that it resolves an unreported issue for in. Currently, in outputs versions in UTC only, even if a location is specified. check and out both leverage the specified locations for formatting purposes. It looks like it has always been that way, going all the way back to the initial commit, so I don't think this fix is going to do much harm.
  • The very last commit in this PR might be a bit scope creepy. In wiring up the new capabilities, I found an easy way to ensure that a version is generated by check for new pipelines, even if/when a start and stop or days are specified. As a result, I think this would resolve some of the questions that come up in issues like Adding a zero time #11 and Can't manually trigger a pipeline outside time range #24.

[edit - more thoughts after I slept on it]

  • The existing contexts in check_command_test.go which define an interval all define an interval of time.Minute. While that certainly makes things simpler, it puts added stress on the contexts in offset_test.go to ensure that values are evenly distributed across an interval, a range, and a range with an interval since there is a specific conditional in place to handle time.Minute intervals.
  • There are a couple of contexts in check_command_test.go that refer to "in the previous time range" vs. "at the end of the previous time range". The subtle difference in phrasing led me to implement them slightly differently. For the contexts that set up supplied versions in the previous range, I calculated the previous version relative to the current version - in other words, the previous version is still valid since it is discoverable by check. For the contexts that set up supplied versions near the end of the previous range, I calculated the previous version relative to now - but those versions are not detectable by check, meaning they don't (or shouldn't) be included in the list of versions provided by check. These near the end of are handy for the purpose of validating how time-resource versions being generated in the wild today will be handled (see my documented assumptions in Make time resource versions more predictable #45 for background).

@vito vito assigned clarafu and vito Sep 3, 2020
@owenfarrell owenfarrell deleted the clock branch December 20, 2020 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make time resource versions more predictable
3 participants