Add Preconditions to Tasks #205

stephenprater · 2019-05-17T20:45:05Z

What changes does this PR introduce?

This adds a preconditions stanza to the task stanza. A precondition is basically the inverse of a status in that it's a condition that must succeed before the task can run.

Any background context you want to provide?

Preconditions on file statuses doesn't make much sense, so preconditions are limited to sh interpreter invocations. Furthermore - preconditions often require the user to do something in order to proceed, so they have a msg field that can be used to report next steps to the user.

I've added an "ignore_error" feature to preconditions, which will ignore errors in preconditions if the task is being executed as either a dependency or as a sub-task. I'm not 100% this isn't a misfeature, but it was requested by our team, so it's there. Normally a failure in a precondition will fail the task that it's on, and thus any "upstream" tasks as well.

Where should the reviewer start?

TestPrecondition enumerates all of the situations I could think of that encounter a conditional - it's a pretty good place to see how to use preconditions - they are executed in the same context as status commands in areTaskPreconditionsMet

Has this been manually tested? How?

We run our own fork of go-task with this feature for our automation. So far it's working well.

andreynering · 2019-05-18T11:47:03Z

Hi @stephenprater,

Thanks! This looks useful, indeed.

I'll review it soon.

smyrman

@andreynering, just looked over this and added some comments, Feel free tom resolve them if you don't agree when you do your review.

internal/taskfile/task.go

smyrman · 2019-05-20T12:37:23Z

internal/taskfile/version/version.go

@@ -10,6 +10,8 @@ var (
 	v21 = mustVersion("2.1")
 	v22 = mustVersion("2.2")
 	v23 = mustVersion("2.3")
+	v24 = mustVersion("2.4")
+	v25 = mustVersion("2.5")


@andreynering. please correct me if I am wrong.

2.4 and 2.5 have been released, so when adding a new feature to the taskfile here, it should probably come as a 2.6 version.

Since there are not checks for 2.4 and 2.5 today, I presume these two releases does not provide any updated to the Taskfile format, and does not need the v2X and Isv2X() definitions?

Yeah, I have forgot to bump these version checks. Let's keep these new variables, and I plan to fix the checks for for the [few] changes we had recently.

This new change should be guarded on v2.6 (the next release), though.

andreynering

Great work overall. 👏

Thanks for adding documentation and tests! 🙂

I want to understand the motivation behind the addition of ignore_error here before continuing.

I've added an "ignore_error" feature to preconditions, which will ignore errors in preconditions if the task is being executed as either a dependency or as a sub-task. I'm not 100% this isn't a misfeature, but it was requested by our team, so it's there. Normally a failure in a precondition will fail the task that it's on, and thus any "upstream" tasks as well.

Can you explain why do you need that? Perhaps we can find a better way of doing this.
I think ignore_error here is a bad name and will confuse users, since it'll only be ignored on sub-calls/deps. If we're going to keep this, we need a better name.

andreynering · 2019-05-25T20:44:48Z

internal/taskfile/version/version.go

@@ -10,6 +10,8 @@ var (
 	v21 = mustVersion("2.1")
 	v22 = mustVersion("2.2")
 	v23 = mustVersion("2.3")
+	v24 = mustVersion("2.4")
+	v25 = mustVersion("2.5")


Yeah, I have forgot to bump these version checks. Let's keep these new variables, and I plan to fix the checks for for the [few] changes we had recently.

This new change should be guarded on v2.6 (the next release), though.

internal/taskfile/task.go

docs/taskfile_versions.md

docs/usage.md

internal/taskfile/precondition.go

task.go

variables.go

stephenprater · 2019-05-28T19:26:21Z

@andreynering We discussed the ignore_errors behavior this morning. We added this as part of adding preconditions to our fork, but we don't actually use it in practice. We're all of the opinion now that it's a misfeature. (IE - A "feature" that works in surprising, confusing or bug-like ways.) I am just going to remove it.

The TL;DR of the discussions was that nobody could think of a single situation in which you would want the behavior of a task not to run because it's preconditions weren't met AND where that failure wouldn't bubble up to a calling task.

I'll make these changes today.

Co-Authored-By: Andrey Nering <andrey.nering@gmail.com>

stephenprater · 2019-05-28T20:09:14Z

task.go

@@ -119,16 +119,17 @@ func (e *Executor) Setup() error {
 			Vars:   e.taskvars,
 			Logger: e.Logger,
 		}
-	case version.IsV2(v), version.IsV21(v), version.IsV22(v):
+	case version.IsV2(v), version.IsV21(v), version.IsV22(v), version.IsV23(v):


@andreynering Should this be changed to version.isV26(v)? I'm a little confused about how this version detection works - since if I change the point version in my actual Taskfiles I get "X is only available on "

minimal repro:

--- version: '2.3' output: prefixed

task: Taskfile option "output" is only available starting on Taskfile version v2.1

This seems like a bug to me in that '2.3' is obviously > 2.1 - but maybe the version checking is only against the major version?

@stephenprater Indeed, the version code needs some love.

In theory this should actually be:

case version.IsV2(v), version.IsV21(v), version.IsV22(v), version.IsV23(v), version.IsV24(v), version.IsV25(v), version.IsV26(v):

But there's likely a way to create a specific constraint for >= 2, < 3.

I'd say, let this as is, and I'll add a reminder to fix this once I merge this PR to master. 🙂

stephenprater · 2019-06-05T19:58:31Z

@andreynering Anything more for me to do on this at this time?

andreynering · 2019-06-06T01:26:57Z

@stephenprater Sorry, I'll review this again once I have available time

stephenprater · 2019-06-06T16:28:53Z

No worries! Just wanted to make sure you weren't waiting on me.

andreynering

Hi @stephenprater,

Thanks once again for this, it was very well implemented. 🙂

This is close to be merged, just made a couple of easy-to-fix comments.

REMINDER TO SELF: Address version check issues once this is merged.

docs/usage.md

precondition.go

task.go

task_test.go

docs/usage.md

internal/taskfile/precondition.go

stephenprater · 2019-06-11T19:25:52Z

@andreynering Changing the extra error logging in task.go to only log at the caller does have a slight behavioral change.

From:

1 != 0 obviously!
task: precondition not met

To:

task: 1 != 0 obviously!

I went ahead and made the change - I think the original output is a little clearer in that it's more obvious why the task failed - but it's also noisier, so 🤷‍♂

andreynering · 2019-06-16T00:55:13Z

@stephenprater Merging this now, and going to fix the version check code.

Thanks again for your work and patience. 🙂

stephenprater mentioned this pull request May 17, 2019

"Required Variables" and results of cmd #169

Closed

stephenprater force-pushed the add-precondition-to-task branch from 2e5a122 to d533aca Compare May 17, 2019 20:48

Add Preconditions to Tasks

bd5882f

stephenprater force-pushed the add-precondition-to-task branch from d533aca to bd5882f Compare May 17, 2019 20:51

smyrman reviewed May 20, 2019

View reviewed changes

andreynering reviewed May 25, 2019

View reviewed changes

stephenprater and others added 2 commits May 28, 2019 12:28

Apply suggestions from code review

659cae6

Co-Authored-By: Andrey Nering <andrey.nering@gmail.com>

Remove ignore_errors

044d3a0

stephenprater commented May 28, 2019

View reviewed changes

Clarify difference between status and precondition in docs

12ab01d

stephenprater force-pushed the add-precondition-to-task branch from 7c768fc to 12ab01d Compare May 28, 2019 20:18

andreynering mentioned this pull request Jun 1, 2019

Feature Request: support ansible style inventory|group|host variables and templating #208

Closed

andreynering reviewed Jun 8, 2019

View reviewed changes

Fix typos per review

d1463b3

stephenprater force-pushed the add-precondition-to-task branch from 65c9cf8 to d1463b3 Compare June 11, 2019 18:46

Change error output

cc92648

andreynering merged commit 0608782 into go-task:master Jun 16, 2019

andreynering added a commit that referenced this pull request Jun 16, 2019

Add changelog for #205

9c68c7c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Preconditions to Tasks #205

Add Preconditions to Tasks #205

stephenprater commented May 17, 2019

andreynering commented May 18, 2019

smyrman left a comment

smyrman May 20, 2019

andreynering May 25, 2019

andreynering left a comment

andreynering May 25, 2019

stephenprater commented May 28, 2019

stephenprater May 28, 2019

stephenprater May 28, 2019

andreynering Jun 8, 2019

stephenprater commented Jun 5, 2019

andreynering commented Jun 6, 2019

stephenprater commented Jun 6, 2019

andreynering left a comment

stephenprater commented Jun 11, 2019

andreynering commented Jun 16, 2019

Add Preconditions to Tasks #205

Add Preconditions to Tasks #205

Conversation

stephenprater commented May 17, 2019

What changes does this PR introduce?

Any background context you want to provide?

Where should the reviewer start?

Has this been manually tested? How?

andreynering commented May 18, 2019

smyrman left a comment

Choose a reason for hiding this comment

smyrman May 20, 2019

Choose a reason for hiding this comment

andreynering May 25, 2019

Choose a reason for hiding this comment

andreynering left a comment

Choose a reason for hiding this comment

andreynering May 25, 2019

Choose a reason for hiding this comment

stephenprater commented May 28, 2019

stephenprater May 28, 2019

Choose a reason for hiding this comment

stephenprater May 28, 2019

Choose a reason for hiding this comment

andreynering Jun 8, 2019

Choose a reason for hiding this comment

stephenprater commented Jun 5, 2019

andreynering commented Jun 6, 2019

stephenprater commented Jun 6, 2019

andreynering left a comment

Choose a reason for hiding this comment

stephenprater commented Jun 11, 2019

andreynering commented Jun 16, 2019