A way to enforce required variables

[benc-uk]

Summary

This is an implementation of this proposal #1203
Which is a way for tasks to validate and check required variables prior to running

Adds

A new field called requires with a sub-field vars is added to the task struct, this field is an array of strings. These strings should be variable names, and these variables are checked for existence before the task is run. If one or more is found to be missing, the task will not run and an error describing the missing variable(s) is returned.

e.g.

version: '3'

tasks:
  docker-build:
    cmds:
      - 'docker build . -t {{.IMAGE_NAME}}:{{.IMAGE_TAG}}'

    # Make sure these variables are set before running
    requires: 
      vars: [IMAGE_NAME, IMAGE_TAG]

I basically copied a lot of what I saw happening in precondition.go so this does add another top level file requires.go to the project. It only has a single function, so this function could moved elsewhere.

Documentation has also been updated

[andreynering]

Nice work @benc-uk! 👏 👏 👏 This is going to be useful.

I added a few simple adjustments to be made before this is ready to be merged. This PR also needs a rebase or merge from main.

In the meantime, I'll also ask a review from @pd93 just to get a second opinion on the syntax.

requires looks great. The only disadvantage I can think of is that we may want to implement the very same thing for envs as well. In that case, maybe we could go with...

requires:
  vars: [FOO, BAR]
  env: [BAZ]

Opinions are welcome.

[danielloader]

I've spent most of my career checking for unset env variables at the start of bash scripts so I'd concur it's a common pattern worth applying to binaries as well as environment variables.

[pd93]

In the meantime, I'll also ask a review from @pd93 just to get a second opinion on the syntax.

requires looks great. The only disadvantage I can think of is that we may want to implement the very same thing for envs as well. In that case, maybe we could go with...

requires:
  vars: [FOO, BAR]
  env: [BAZ]

Opinions are welcome.

My issue with requires is that its not very specific for a top-level field. If you gave me no docs, I could interpret the word requires to mean any number of things in relation to a task (i.e. could be confused with deps/prerequisites etc.). requires_vars is more specific, but not very "Task-like" (too long & don't like the hyphen). So it seems that...

requires:
  vars: []

...fulfils both the need for specificity, allows us to require other things (like envs as @andreynering said) later and also still feels "Task-like".

[benc-uk]

I've spent most of my career checking for unset env variables at the start of bash scripts so I'd concur it's a common pattern worth applying to binaries as well as environment variables.

Do you mean checking that various binaries are in the system path? I was considering adding this too, as I've also spent far too much time doing which foo > /dev/null || { echo "Error! Command foo not installed"; exit 1; } in hundreds of bash scripts.

I wanted to see how this initial proposal and change went before adding more features like this

[benc-uk]

@pd93 @andreynering

PR updated with all suggestions, and docs, schema, error codes updated too.

The current implementation matches the suggestion, e.g.

requires:
  vars: []

As for separating check for env vs vars, I'm not sure there's much reason or advantage, as tasks blend the two into a "bag" of variables that can be referenced interchangeably. That's what I'm checking against e.g. Compiler.GetVariables which seems to be a big map of both env and vars combined together

[benc-uk]

Note some reformatting crept in when I saved the file due to my VSCode settings, but the changes looked pretty sensible So I kept them

[pd93]

As for separating check for env vs vars, I'm not sure there's much reason or advantage, as tasks blend the two into a "bag" of variables that can be referenced interchangeably.

There's been a lot of confusion in the past over the fact that envs can be accessed using Go templating as if they are variables. This may change in the future, so it might be nice to add the requires.envs check down the line. I don't think this is needed for this PR to be merged though.

[pd93]

Checked out the code and everything seems to work well. I'm happy with the changes that have been made. It might be nice to add a test either in task_test.go or create a requires_test.go, but other than that, it LGTM.

[benc-uk]

There's been a lot of confusion in the past over the fact that envs can be accessed using Go templating as if they are variables. This may change in the future, so it might be nice to add the requires.envs check down the line. I don't think this is needed for this PR to be merged though.

Yes this confused me when I first started using Task, I didn't realize it was a by-product of Go templating.

I actually like the flexibility it provides. For example GitHub Actions has very specific syntax ${{ env.FOO }}, but also $FOO is supported of course if the step is bash, then they added "configuration variables" so there's a ${{ var.BAR }} now too.

I guess what I'm saying is, this is surprisingly hard to get right and find the balance between verbosity and clarity

[treybrisbane]

Has there been any thought into defining optionality within variable definitions themselves?
E.g.

version: '3'

vars:
  NAME:
    required: true

You could actually take it slightly further and include a default keyword as well:

version: '3'

vars:
  NAME:
    required: true
  VERSION:
    required: false
    default: latest

(Granted, you can already use the default Slim-Sprig function for this - this is mostly just a concept.)

It could also pair nicely with dynamic variables to require that the shell command actually outputs something:

version: '3'

vars:
  NAME:
    sh: find . -type f -name package.json | head -n 1
    required: true

[pd93]

Has there been any thought into defining optionality within variable definitions themselves?

@treybrisbane I actually do quite like your proposed syntax. The only disadvantage I can see is that its quite a bit more verbose (in terms of lines consumed):

version: '3'

vars:
  FOO:
    required: true
  BAR:
    required: true

tasks:
  foo:
    cmds:
      - ...

As opposed to:

version: '3'

tasks:
  foo:
    requires:
      vars: ['FOO', 'BAR']
    cmds:
      - ...

However, being able to set variable requirements at a global-level and at task-level is quite powerful and it does keep related stuff together in the definition. I think this is worth considering before releasing. @andreynering @benc-uk WDYT?

[benc-uk]

Has there been any thought into defining optionality within variable definitions themselves?

@treybrisbane I actually do quite like your proposed syntax. The only disadvantage I can see is that its quite a bit more verbose (in terms of lines consumed):

version: '3'

vars:
  FOO:
    required: true
  BAR:
    required: true

tasks:
  foo:
    cmds:
      - ...

As opposed to:

version: '3'

tasks:
  foo:
    requires:
      vars: ['FOO', 'BAR']
    cmds:
      - ...

However, being able to set variable requirements at a global-level and at task-level is quite powerful and it does keep related stuff together in the definition. I think this is worth considering before releasing. @andreynering @benc-uk WDYT?

I think this is potentially a good idea, but it doesn't replace the "per task" level required vars, as a project often needs a mix.

For example if I'm running task lint it probably doesn't matter if the DOCKER_TAG var isn't set and it feels weird forcing people to set it, but it's a different story if I'm running task build-img

Personally I'd add taskfile or global level required vars as a separate PR

[pd93]

I think this is potentially a good idea, but it doesn't replace the "per task" level required vars, as a project often needs a mix.

For example if I'm running task lint it probably doesn't matter if the DOCKER_TAG var isn't set and it feels weird forcing people to set it, but it's a different story if I'm running task build-img

Personally I'd add taskfile or global level required vars as a separate PR

To be clear, I think the syntax proposed by @treybrisbane could be used at a global and a task-level:

version: '3'

vars:
  FOO:
    required: true
  BAR:
    required: true

tasks:
  task-a:
    vars:
      BAZ:
        required: true
    cmds:
      - ...

This Taskfile would always require FOO and BAR for all tasks, but task-a would also specifically require BAZ to be set. The vars keyword is already implemented at both levels anyway, so if we were to use this syntax, we'd definitely want it to be the same at both levels.

[andreynering]

Hi guys, sorry for the long time to respond. I may have some time this week to review and merge.

I think I prefer requires: the way @benc-uk implemented. It looks cleaner to me, and is less verbose. If you have multiple required variables, it takes only 1 line instead of one for each variable.

I think this is probably easier to implement as well, and less likely to be a shoot in the foot when v4 comes and we're going to change some things about how variables and envs work.

Regarding requiring for all tasks, nothing stops us to allow requires: in the root of the Taskfike as well, if we think it's needed. Can be done in the future if people ask for it IMO.

Looks good @pd93?

[pd93]

@andreynering Yeah, I'm still fine with this as it is. Just worth considering the alternatives before merging. I do think that adding this at a global-level will be useful at some point, but doesn't need to be now.

[treybrisbane]

@andreynering Thanks for considering. 🙂

If I may, I'd like to make some final arguments in favour of the approach I suggested:

---

Consistency

Scoping required under variables is consistent with how Task scopes other properties under the element to which they relate.

To that point, let's look at sh. sh is a child of a variable:

vars:
  NAME:
    sh: whoami

It's not a sibling:

vars: {}
sh:
  NAME: whoami

Similarly, we have silent. silent is a child of the entity to which it relates:

tasks:
  print-full-name:
    cmds:
      - cmd: echo 'Hayley'
        silent: false
      - cmd: echo 'Williams'
        silent: true
    silent: true

It's not a sibling:

tasks:
  print-full-name:
    cmds:
      - echo 'Hayley'
      - echo 'Williams'
    silent: [false, true]
silent:
  print-full-name: true

To me, it naturally follows that required should be a child of a variable:

vars:
  NAME:
    required: true

Not a sibling:

vars: {}
requires:
  vars: ['NAME']

Complexity

Task currently has two ways of declaring variables: vars and env. vars and env have different semantics, yet users sometimes confuse details about the two.

Introducing requires as a separate parameter would mean there would be four ways of declaring variables: vars, env, requires.vars and requires.env. Granted, the semantics of each one are different, but they're similar enough for this to feel unnecessarily complex.

In contrast, introducing required as a variable attribute means that we'd be extending the current variable declaration methods rather than introducing new ones. I'd argue that this is simpler and less complex than the alternative.

Flexibility

Scoping required under variables offers more flexibility going forward compared to having it be a sibling.

To that point, I'd like to present the following concepts:

vars:
  VERSION:
    required: false
    default: latest

vars:
  PACKAGE_JSON_PATH:
    sh: find . -type f -name package.json | head -n 1
    required: true

vars:
  REQUIRED_BOOLEAN:
    type: boolean
    required: true
  OPTIONAL_NON_NEGATIVE_INTEGER:
    type: integer
    minimum: 0
    required: false
    default: 0
  REQUIRED_DOUBLE_ARRAY:
    type: array
    items:
      type: double
    required: true
  OPTIONAL_OBJECT_REQUIRED_FIELDS:
    type: object
    properties:
      name:
        type: string
        required: true
      age:
        type: integer
        required: true
    required: false

I want to stress that these are only concepts (not proposals), but my hope is that they illustrate that nesting required under variables furthers a precedent that empowers numerous other potential features.

---

Ultimately, I think either approach will be a net win for Task. I do feel like the nested approach will be better both now and in the long run, but I won't be sad if you decide to go with the sibling approach. Either way, it'll be nice to declare required variables without needing to use Go templates! 😄

[andreynering]

As always, there are different opinions on the different possible paths to accomplish something. I value everyone's input, but I'm going to proceed with how it was implemented here.

Thanks @benc-uk for your contribution and patience! 🙌