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

Empty required variable still passing the required check? #1676

Closed
trulede opened this issue Jun 1, 2024 Discussed in #1620 · 2 comments
Closed

Empty required variable still passing the required check? #1676

trulede opened this issue Jun 1, 2024 Discussed in #1620 · 2 comments

Comments

@trulede
Copy link

trulede commented Jun 1, 2024

Discussed in #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:

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?

@task-bot task-bot added the state: needs triage Waiting to be triaged by a maintainer. label Jun 1, 2024
@trulede
Copy link
Author

trulede commented Jun 1, 2024

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
Copy link
Author

trulede commented Jun 1, 2024

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).

@trulede trulede closed this as completed Oct 1, 2024
@task-bot task-bot removed the state: needs triage Waiting to be triaged by a maintainer. label Oct 1, 2024
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 a pull request may close this issue.

2 participants