search

concourse / time-resource

a resource for triggering on an interval
Apache License 2.0
44 stars 33 forks source link

Add after time functionality #41

Closed radito3 closed 5 years ago

radito3 commented 5 years ago

Hey, I'm new to Concourse but I wanted to enhance the time resource to add functionality for triggering a task once after a set delay of time. I'm not 100% sure this change will do that, so I would be very grateful if you point me in the direction of the resource or usage of the time resource I need. The case I'm working on consists of two tasks that need to be separated by a 10 minute gap. I need the second one to start 10m after the first is complete without using sleep in the scripts. I think this new parameter I'm adding to the source config will do this but, again, am not completely sure. Thank you.

vito commented 5 years ago

Are you sure you mean task? Do you mean 'job'? :thinking:

radito3 commented 5 years ago

In the first stage it was a 'task' but then I realized resources could only trigger jobs, so now, yes. I'm sorry for my mistake.

owenfarrell commented 5 years ago

So I'm in the same boat - looking for a mechanism to inject a delay in to a pipeline. But I took a slightly different tact.

Rather than inject this functionality in to the check mechnaism, I added a parameter to the OutRequest. By parameterizing the delay in a step, someone could leverage a single source definiton at multiple points in a pipeline to inject a delay with different delay intervals.

The actual delay takes place as part of the in function, effectively blocking until the specified version (time) is reached. This maintains backwards compatibility with the existing in function, as a version specified as a result of a return from check should always be instantaneous. But when called in as an implicit get following a put step with a delay parameter (above) actually triggers a delay.

Thoughts on this approach?

radito3 commented 5 years ago

This is exactly what I need. Although I was experimenting with ways of avoiding using 'sleep', nothing worked. And I didn't think about backwards compatibility. Thank you for your suggestion and for your approach.

On another note, should I close this pull request or should I modify my solution to be like the one proposed?

owenfarrell commented 5 years ago

Yeah, I struggled with that myself. By making this synchronous, someone could declare a really long delay interval (increasing the chance of an underlying failure).

But by integrating this function in to the source, I think you increase the number of check containers that have to be managed.

So for the happy path, short(ish) delay scenario, I think this approach works well. Longer delays probably need less precision, so the existing functionally is perfect for those scenarios. If you need a long delay and precision, you would need a more rich integration with some kind of storage - akin to the gate resource.

radito3 commented 5 years ago

Update, I tested it with your suggestion but it slept twice. First, I think it's the implicit get after a put, then the second time in the job it was supposed to trigger. I moved the after parameter from source to outParams, like in your solution. I moved the sleep call to the out script. I don't know if this is the correct choice but it will mitigate the double execution pausing.

owenfarrell commented 5 years ago

First, I think it's the implicit get after a put, then the second time in the job it was supposed to trigger.

That's odd. I wasn't running in to any kind of delay when running the unit tests.

I just opened #43 which includes another unit test for in and an updated the README. Do you have a pipeline that you're using to test with?

radito3 commented 5 years ago

Yes, I do but it's company private, not my own. Though I just read the comments in your pull request and will probably use @vito 's solution for creating a sleep task, since the builder task he referenced won't be of use to me. I am thankful for your feedback on this issue. I would want to hear @vito 's feedback on this, should I close this pull request or will he do it, since I understood the entirety of the problem I was trying to solve and no longer need to make this change to the time resource.

vito commented 5 years ago

@radito3 Let's just close this. :slightly_smiling_face: Adding a delay/sleep feels like more of a task thing, since it's actual run-time behaviour that you're after in this case.

Thanks for the PR though!