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

Fix and modify requires behavior. #1677

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

trulede
Copy link

@trulede trulede commented Jun 1, 2024

Fix and modify the behavior of requires.

Fixes #1676 (discussion #1620)
Modifies/updates #1204

Requires was not working in the case that a variable was set to nil (see discussion/issue).

Additional, the behavior of requires is modified so that an zero length (empty) string value fails the required check. In more complex use cases, with sub-tasks and variable templates, the expectation is that requires will ensure that a variable is set to something. In such cases, an empty string is not considered as something and so the feature is not useful.

An alternative would be to have a way to set a variable value to nil via the template mechanism.

The change is tested with the following taskfile (modified from the original discussion).

version: "3"

silent: true

vars:
  IMAGE_NAME: my-web-app
  CONTAINER_REGISTRY: 
  # CONTAINER_REGISTRY: # comment
  # CONTAINER_REGISTRY: ''
  # CONTAINER_REGISTRY: ' '

tasks:
  check-missing:
    desc: Test the task variables
    requires:
      vars: [CONTAINER_REGISTRY]
    cmds:
      - echo "CONTAINER_REGISTRY is {{spew .CONTAINER_REGISTRY}}"

@trulede
Copy link
Author

trulede commented Jun 2, 2024

Further to the change in behaviour, please see the results of this experiment where the templating system is considering a zero length string as "not set".

version: "3"

vars:
  A: fubar
  B: ''
  C:

tasks:
  check-set:
    desc: Test the task variables
    vars:
      A_VAR:  '{{.A | default "A NOT SET"}}'
      B_VAR:  '{{.B | default "B NOT SET"}}'
      C_VAR:  '{{.C | default "C NOT SET"}}'
    cmds:
      - echo "A is {{.A_VAR}}"
      - echo "B is {{.B_VAR}}"
      - echo "C is {{.C_VAR}}"

produces this output

$ task check-set -f -v
task: [/mnt/c/Users/trule] Not found - Using alternative (Taskfile.yml)
task: "check-set" started
task: [check-set] echo "A is fubar"
A is fubar
task: [check-set] echo "B is B NOT SET"
B is B NOT SET
task: [check-set] echo "C is C NOT SET"
C is C NOT SET
task: "check-set" finished

This PR modifies the requires behaviour to match that of the templating system, which is I think essential for requires to be usable. Currently, we do this FOO: '{{.FOO| default "FOO_NOT_SPECIFIED"}}' and hope that the user will notice the strange message in the output ... we tried requires, but it did not work. The concise error message that requires produces would be very welcome, both for developers and users.

@andreynering andreynering self-requested a review July 28, 2024 11:55
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 this pull request may close these issues.

Empty required variable still passing the required check?
1 participant