go-task / task

A task runner / simpler Make alternative written in Go
https://taskfile.dev
MIT License
10.59k stars 561 forks source link

Empty required variable still passing the required check? #1676

Open trulede opened 1 month ago

trulede commented 1 month ago

Discussed in https://github.com/go-task/task/discussions/1620

Originally posted by **usersina** April 26, 2024 - Task version: `v3.36.0` - Operating system: `Arch Linux x86_64 6.6.28-1-lts` - Experiments enabled: `No` Given the following `Taskfile.yml`: ```yml version: "3" silent: true vars: IMAGE_NAME: my-web-app CONTAINER_REGISTRY: # docker.io tasks: check-missing: desc: Test the task variables requires: vars: [CONTAINER_REGISTRY] cmds: - echo "CONTAINER_REGISTRY is {{.CONTAINER_REGISTRY}}" ``` My `check-missing` is not working as intended. Actual output > CONTAINER_REGISTRY is Expected output > task: Task "check-missing" cancelled because it is missing required variables: CONTAINER_REGISTRY This works as intended if I simply delete `CONTAINER_REGISTRY` or comment it out, but why isn't it working if it's empty?
trulede commented 1 month ago

Seems like the the code is only checking if the variable exists, not if it is also set.

Expectation is that nil or "" (empty string) would cause such a variable to fail the requires checks.

trulede commented 1 month ago

Seems like the the code is only checking if the variable exists, not if it is also set.

Expectation is that nil or "" (empty string) would cause such a variable to fail the requires checks.

Documentation has this:

Variables set to empty zero length strings, will pass the requires check.

which is not so good. I really have to question that, both in what it means (what is an empty and zero length string), and if such behavior is even useful. The code behind variables is quite involved, so in many places you find variables being defaulted to an empty string (i.e. "") and other things.

Again with the documentation:

If you want to check that certain variables are set before running a task then you can use requires. This is useful when might not be clear to users which variables are needed, or if you want clear message about what is required. Also some tasks could have dangerous side effects if run with un-set variables.

which seems OK, but misses the use-cases where one task calls another with non-trivial variable passing, and included task files ... these are cases where requires would be useful. We have it in our task files, but it only acts as documentation for us (since it does not work).

I think the required checks should be:

  1. Does the variable exist (in the scope of a compiled task).
  2. Is the variable set.
  3. Is the variable set to a string of length greater than 0.

In the case that 3 would not be accepted as part of the implementation, then we need a way to set a variable to nil via the template engine (e.g a slim sprig function).