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

Any variables experiment #1415

Closed
3 tasks
pd93 opened this issue Nov 30, 2023 · 27 comments
Closed
3 tasks

Any variables experiment #1415

pd93 opened this issue Nov 30, 2023 · 27 comments
Labels
area: variables Changes related to variables. experiment: released Experimental feature - Now the default behavior in Task and is no longer an experiment.

Comments

@pd93
Copy link
Member

pd93 commented Nov 30, 2023

Note

This experiment has been released and is now generally available since Task v3.37.0. This issue is kept for historical purposes only. For up-to-date information, you can read our blog post or check the variables documentation.

Context

This experiment attempts to solve the problems originally described by #140.

Currently, all Task variables are strings. You can define a variable as a YAML string, bool, int, float or null value. However, when task interprets it, it will be decoded as a string regardless of the input type.

For example, the following Taskfile will always output "foo" even when BOOL is false because the variable is being interpreted as a string and an if statement evaluates to true when the string is not empty:

version: 3

tasks:
  foo:
    vars:
      BOOL: false
    cmds:
      - '{{ if .BOOL }}echo foo{{ end}}'

Lists are also interpreted as strings and this has led to some annoying workarounds in the codebase. For example, the for feature is great for running a task multiple times with different variables. However, if you want to loop over an arbitrary list of strings, you have to define the list as a delimiter separated string and then split it by specifying the delimiter in the for statement. For example:

version: 3

tasks:
  foo:
    vars:
      LIST: 'foo,bar,baz'
    cmds:
      - for:
          var: LIST
          split: ','
        cmd: echo {{ .ITEM }}

This could be simplified if we supported variables as lists:

version: 3

tasks:
  foo:
    vars:
      LIST: [foo, bar, baz]
    cmds:
      - for:
          var: LIST
        cmd: echo {{ .ITEM }}

Proposal

We propose to change the type of internal Task variables to any (otherwise known as an empty interface{} in Go). This will allow users to define variables as any type they want, and Task will interpret them properly when used in tasks. The following types should be supported:

  • string
  • bool
  • int
  • float
  • array
  • map

Adding support for these types is relatively simple by itself. However, there a few changes that will be needed to make the rest of Task's features work nicely with the new variable types:

Backwards Compatibility

The current implementation of Task variables allows for dynamic variables to be specified by using the sh subkey. For example:

version: 3

task:
  foo:
    vars:
      CALCULATED_VAR:
        sh: 'echo hello'
    cmds:
      - 'echo {{ .CALCULATED_VAR }}'

Running task foo will output the following:

task: [foo] echo hello
hello

Since we are adding support for map variables, this syntax will conflict and can no longer be supported. Instead, we should detect string variables that begin with a $ and interpret them as a command to run. For example:

version: 3

task:
  foo:
    vars:
      CALCULATED_VAR: '$echo hello'
    cmds:
      - 'echo {{ .CALCULATED_VAR }}'

If a user wants a string variable that starts with $, they can escape it with a backslash: \$.

Removing the sh subkey will break any Taskfiles that currently use it and it is possible that the new syntax will also break existing taskfiles that have variables that start with $. For this reason, the functionality in this proposal will stay as an experiment until at least the next major version of Task.

@task-bot task-bot added the state: needs triage Waiting to be triaged by a maintainer. label Nov 30, 2023
@pd93 pd93 added experiment: proposed Experimental feature - Pending feedback on proposal. and removed state: needs triage Waiting to be triaged by a maintainer. labels Nov 30, 2023
@task-bot
Copy link
Collaborator

This issue has been marked as an experiment proposal! 🧪 It will now enter a period of consultation during which we encourage the community to provide feedback on the proposed design. Please see the experiment workflow documentation for more information on how we release experiments.

@pd93 pd93 added the breaking change Changes that are not backward compatible. label Nov 30, 2023
@pd93 pd93 moved this to Proposed in Task Experiments Nov 30, 2023
@andreynering
Copy link
Member

Thanks for opening this issue!

Some small comments:

  • CLI_ARGS is intentionally a string, so it's as easy as possible to delegate arguments like yarn {{.CLI_ARGS}}. We should keep it as a string for this use case, but we can set an additional var which would be an array.
  • There's an irony of suggesting $ as a prefix for variables, because that's how Task detected commands in the beginning, but it was eventually deprecated in favor of sh: 🙂. We would be returning to its origins. Perhaps there's a better than way than that prefix, but I'm out of ideas at the moment.

@pd93
Copy link
Member Author

pd93 commented Nov 30, 2023

  • CLI_ARGS is intentionally a string, so it's as easy as possible to delegate arguments like yarn {{.CLI_ARGS}}. We should keep it as a string for this use case, but we can set an additional var which would be an array.

Ok, that makes sense. I'll remove this section for now. We can revisit down the line if necessary. This reduces the amount of breaking changes.

  • There's an irony of suggesting $ as a prefix for variables, because that's how Task detected commands in the beginning, but it was eventually deprecated in favor of sh: 🙂. We would be returning to its origins. Perhaps there's a better than way than that prefix, but I'm out of ideas at the moment.

I don't think I knew about Task in the days before sh, but I actually agree. I'm not convinced by this syntax and it seems like it could cause problems/confusion. However, I'm not entirely sure what a good alternative is either. I suggested this approach as there appeared to be some consensus in #140. However, the comments there are quite old, so perhaps a fresh think about this is in order.

There is always the option of not supporting maps initially. This would actually mean that we could release this without a major version bump as I don't think there would be any remaining backward-incompatible changes.

@pd93 pd93 linked a pull request Dec 3, 2023 that will close this issue
@pd93 pd93 removed a link to a pull request Dec 3, 2023
@andreynering andreynering linked a pull request Dec 21, 2023 that will close this issue
@andreynering andreynering removed a link to a pull request Dec 21, 2023
@pd93 pd93 added experiment: draft Experimental feature - Pending feedback on draft implementation. and removed experiment: proposed Experimental feature - Pending feedback on proposal. labels Dec 21, 2023
@pd93 pd93 moved this from Proposed to Draft in Task Experiments Dec 21, 2023
@task-bot
Copy link
Collaborator

task-bot commented Dec 21, 2023

This experiment has been marked as a draft! ✨ This means that an initial implementation has been added to the latest release of Task! You can find information about this experiment and how to enable it in our experiments documentation. Please see the experiment workflow documentation for more information on how we release experiments.

@vmaerten
Copy link
Member

Hi !
Thanks a lot for your work on it! It's really appreciated. I will give it a try, because map is something I wanted to use.
I just see you made a PR about looping over map, that's also a great thing!

If you are interessed in, I can post a use case of it

@toby-griffiths
Copy link

Thanks for the great work on Taskfile.

I'm having issues suporting proposal 2 (not tested porposal 1 yet.

Here's my Taskfile, containing one of the examples given…

version: '3'

tasks:
  foo:
    desc: test
    vars:
      FOO:
        map: { a: 1, b: 2, c: 3 } # <-- Defined using the `map' subkey instead of directly on 'FOO'
      BAR: true # <-- Other types of variables are still defined directly on the key
      BAZ:
        sh: 'echo Hello Task' # <-- The `sh` subkey is still supported
    cmds:
      - 'echo {{.FOO.a}}'

When I attempt to call task --list-all I get the following error…

template: :1:11: executing "" at <.FOO.a>: can't evaluate field a in type interface {}

Task version: v3.33.1 (h1:JJSRANHH7RQrr5Z2CTvSnTH7iWlfBlKV2W2O0JiZoLk=)

And here's my .env file in the same directory…

TASK_X_ANY_VARIABLES=2

What am I doing wrong?

@pd93
Copy link
Member Author

pd93 commented Jan 17, 2024

Hi @toby-griffiths.

Firstly, thanks for trying this out. Definitely looking for some feedback on these experiments.

Secondly, apologies for any confusion. v3.33.1 does not contain support for the 2nd proposal and this change has not been included in any release yet. Unfortunately, we still haven't versioned our docs, so they are more up to date than Task itself (This is something we need to resolve).

If you are using Go to install Task, then you can run go install github.com/go-task/task/v3/cmd/task@main to grab the latest commit from main. This will allow you to try it out before we do the next release. You can verify that the proposal experiment is enabled by running task --experiments and you should see this:

* GENTLE_FORCE:     off
* REMOTE_TASKFILES: off
* ANY_VARIABLES:    on (2)

Just be aware that unreleased versions (particularly experiments) might be a bit buggy. Not a problem if you're just trying stuff out though.

@toby-griffiths
Copy link

Ah, thanks @pd93 I'll try installing via go (I had used Homebrew).

In case it's useful info, I've also just tried the following proposal 1 example…

version: '3'

tasks:
  foo:
    vars:
      ARRAY: [ 1, 2, 3 ]
    cmds:
      - 'echo {{range .ARRAY}}{{.}}{{end}}'

… to receive the error…

yaml: line 13: cannot unmarshal !!seq into variable

So not sure if this is related or a separate issue?

@pd93
Copy link
Member Author

pd93 commented Jan 17, 2024

@toby-griffiths Your example works for me on v3.33.1.

❯ TASK_X_ANY_VARIABLES=1 task foo 
task: [foo] echo 123
123

The error you're getting is symptomatic of the experiment not being enabled, so I suspect that we're not reading your .env file correctly. You can run task --experiments to check this.

Are you by any chance using the --taskfile or --dir flags in the command that you're running? Where is your .env file located relative to your Taskfile.yml? and what directory are you in when you run the command relative to your Taskfile.yml?

@toby-griffiths
Copy link

toby-griffiths commented Jan 17, 2024

@pd93 So I checked and it reported that the experiment was there an enabled, yet I got that error.

I've since installed the Go version and it's working fine with both proposales.

I also found that spew didn't seem to work with the Homebrew version, so I wonder if that's just not all there, somehow?

I wasn't using --dir or --taskfile when calling task, and the .env file was in the same directory as the Taskfile.yaml.

I'm using .yaml rather than .yml, but that shouldn't matter, right?

@pd93
Copy link
Member Author

pd93 commented Jan 17, 2024

@pd93 So I checked and it reported that the experiment was there an enabled, yet I got that error.

Ah, I just realised, this is probably related to #1463 which would explain why it's fixed when you tried main. This will be resolved in the next version.

I also found that spew didn't seem to work with the Homebrew version, so I wonder if that's just not all there, somehow?

spew will be in the next release too.

I'm using .yaml rather than .yml, but that shouldn't matter, right?

This shouldn't make a difference. They are equivalent.

I've since installed the Go version and it's working fine with both proposales.

Please let us know how you find them!

@cwrau
Copy link

cwrau commented Feb 19, 2024

Is it wanted behaviour, that nested keys in objects aren't templated?

And how can I set a nested variable from the outside, e.g. via environment variables or some kind of flag?

@pd93
Copy link
Member Author

pd93 commented Feb 19, 2024

Is it wanted behaviour, that nested keys in objects aren't templated?

Hi @cwrau, could you please provide an example Taskfile with what you're seeing and what you expected to see. This will help me better understand what the problem you're having is.

And how can I set a nested variable from the outside, e.g. via environment variables or some kind of flag?

This is outside of the scope of this experiment. Currently only string types are supported from environment variables. However, I'd be open to proposals on how this could work in a future improvement!

@cwrau
Copy link

cwrau commented Feb 19, 2024

Is it wanted behaviour, that nested keys in objects aren't templated?

Hi @cwrau, could you please provide an example Taskfile with what you're seeing and what you expected to see. This will help me better understand what the problem you're having is.

vars:
  image:
    tag: '{{ .IMAGE_TAG | default "latest" }}'

When used later, it just contains the string {{ .IMAGE_TAG | default "latest" }}

@pd93
Copy link
Member Author

pd93 commented Mar 3, 2024

@cwrau @toby-griffiths If you have some time, please try out the changes #1526. They should address your comments/issues. If you have Go installed, you can try the version in the PR by running:

go install github.com/go-task/task/v3/cmd/task@4bf9c78

Also, if you have any other general feedback on the experiments, I'd be very keen to hear it. Particularly with regard to the two different proposals and how they handle map variables. Thanks

@cwrau
Copy link

cwrau commented Mar 4, 2024

@cwrau @toby-griffiths If you have some time, please try out the changes #1526. They should address your comments/issues. If you have Go installed, you can try the version in the PR by running:

go install github.com/go-task/task/v3/cmd/task@4bf9c78

Mh, doesn't appear to be working?

version: '3'
vars:
  cheese:
    cake: chocolate
  cheesecake: chocolate
tasks:
  not-working:
    cmd: echo {{ printf "%s" .cheese.cake }}
  working:
    cmd: echo {{ printf "%s" .cheesecake }}
$ TASK_X_ANY_VARIABLES=1 task not-working
task: [not-working] echo %!s(*string=0xc00021ac40)
$ TASK_X_ANY_VARIABLES=1 task working
task: [working] echo chocolate

Also, if you have any other general feedback on the experiments, I'd be very keen to hear it. Particularly with regard to the two different proposals and how they handle map variables. Thanks

I'm definitely in favor of variant 1, it just seems more natural to define variables that way and dynamic variables are a special case, so prepending $ also kinda makes sense

@pd93
Copy link
Member Author

pd93 commented Mar 4, 2024

@cwrau Thanks for testing. I made a small mistake in my implementation. It worked when using a variable directly (e.g. {{.cheese.cake}}) so I assumed everything was ok, but nested values were being returned as pointers instead of values so when printing .cheese.cake the type was a *string instead of a string - hence the printf error.

Can you please try again with: go install github.com/go-task/task/v3/cmd/task@3109a89

@cwrau
Copy link

cwrau commented Mar 5, 2024

@cwrau Thanks for testing. I made a small mistake in my implementation. It worked when using a variable directly (e.g. {{.cheese.cake}}) so I assumed everything was ok, but nested values were being returned as pointers instead of values so when printing .cheese.cake the type was a *string instead of a string - hence the printf error.

Can you please try again with: go install github.com/go-task/task/v3/cmd/task@3109a89

That's now working, but the following isn't;

version: '3'
vars:
  cheese:
    cake: chocolate
    type: '{{ .cheese.cake }}'
  cheesecake: chocolate
  cheese_type: '{{ .cheesecake }}'
tasks:
  not-working:
    cmd: echo {{ printf "%s" .cheese.type }}
  working:
    cmd: echo {{ printf "%s" .cheese_type }}
$ TASK_X_ANY_VARIABLES=1 task working
task: [working] echo chocolate
chocolate
$ TASK_X_ANY_VARIABLES=1 task not-working
task: [not-working] echo 

@pd93
Copy link
Member Author

pd93 commented Mar 10, 2024

@cwrau Thanks again. I've merged the changes in #1526 as I think the functionality added there is useful by itself. It's a bit more complicated to do what you've described in #1415 (comment), so I've opened #1544 to track this separately. Let's move any further discussion about this functionality to that issue.

@cwrau
Copy link

cwrau commented Mar 25, 2024

I think I found another bug?;

version: '3'

vars:
  tools:
    controller-gen:
      url: sigs.k8s.io/controller-tools/cmd/controller-gen
      version: 0.14.0

tasks:
  test:
    vars:
      tool: '{{ index .tools "controller-gen" }}'
      type: '{{ typeOf (index .tools "controller-gen") }}'
    cmds:
      - echo "'{{ .type }}' should be map[string]interface {}"
      - echo "'{{ typeOf .tool }}' should also be map[string]interface {}"
$ TASK_X_ANY_VARIABLES=1 task test
task: [test] echo "'map[string]interface {}' should be map[string]interface {}"
'map[string]interface {}' should be map[string]interface {}
task: [test] echo "'string' should also be map[string]interface {}"
'string' should also be map[string]interface {}

This prevents us from pulling in the tool and then using it, instead we need to index tools over and over again

@pd93
Copy link
Member Author

pd93 commented Mar 25, 2024

@cwrau This is expected behaviour. The output type of a Go template will always be a string. It is not possible to avoid this as it is built into Go's templating engine. The 2nd proposal allows you to pass variables by reference, but there is no way to do this in proposal 1 currently.


Edit: I'm still not convinced by the $ syntax for dynamic variables in proposal 1, but if we ended up going with this, maybe we could have something similar for variable references? Something like a ! or # prefix.

@cwrau
Copy link

cwrau commented Mar 25, 2024

@cwrau This is expected behaviour. The output type of a Go template will always be a string. It is not possible to avoid this as it is built into Go's templating engine. The 2nd proposal allows you to pass variables by reference, but there is no way to do this in proposal 1 currently.

to be more specific, I'm trying to accomplish the following;

version: '3'

vars:
  tools:
    controller-gen:
      url: sigs.k8s.io/controller-tools/cmd/controller-gen
      version: 0.14.0

tasks:
  test:
    vars:
      tool: '{{ index .tools "controller-gen" }}'
    cmds:
      - echo "'{{ .tool.url }}' should be 'sigs.k8s.io/controller-tools/cmd/controller-gen'"

but this results in;

$ task test
template: :1:3: executing "" at <index .tools "controller-gen">: error calling index: index of untyped nil

Is there currently a way?

@pd93
Copy link
Member Author

pd93 commented Mar 25, 2024

Is there currently a way?

@cwrau A couple of things here. Firstly, it looks like the error you got was because the experiment wasn't enabled. If I use TASK_X_ANY_VARIABLES=1 task test with your example, I get a different error:

template: :1:15: executing "" at <.tool.url>: can't evaluate field url in type interface {}

This error occurs because the field .url does not exist on the variable .tool because it is a string, not a map.

When you do:

vars:
  tool: '{{ .tools.controller_gen }}'

This is not creating a variable of type map[string]any as you're expecting. As I said previously, all templates output strings, so Go is stringifying the map into something that looks like this: "map[url:sigs.k8s.io/controller-tools/cmd/controller-gen version:0.14.0]".

With TASK_X_ANY_VARIABLES=1 there is no way to pass variables between tasks without converting them to strings first. This is a shortcoming of the templating system. Your only options would be to use the top-level variables directly:

tasks:
  test:
    cmds:
      - echo '{{ .tools.controller_gen.url }}'

or redefine the bottom-level string values you want:

tasks:
  test:
    vars:
      url: '{{ .tools.controller_gen.url }}'
      version: '{{ .tools.controller_gen.version }}'
    cmds:
      - echo "'{{ .url }}' should be 'sigs.k8s.io/controller-tools/cmd/controller-gen'"
      - echo "'{{ .vesrion }}' should be '0.14.0'"

However, with TASK_X_ANY_VARIABLES=2, you are able to pass variables by ref:

version: '3'

vars:
  tools:
    map:
      controller-gen:
        url: sigs.k8s.io/controller-tools/cmd/controller-gen
        version: 0.14.0

tasks:
  test:
    vars:
      tool:
        ref: tools.controller-gen
    cmds:
      - echo "'{{ .tool.url }}' should be 'sigs.k8s.io/controller-tools/cmd/controller-gen'"

Though, as I paste this example, I've realised that it's not currently working 😆 I will look into this. I imagine the syntax for passing by reference with proposal 1 could look something like this:

version: '3'

vars:
  tools:
    controller-gen:
      url: sigs.k8s.io/controller-tools/cmd/controller-gen
      version: 0.14.0

tasks:
  test:
    vars:
      tool: !tools.controller-gen
    cmds:
      - echo "'{{ .tool.url }}' should be 'sigs.k8s.io/controller-tools/cmd/controller-gen'"

... but this does not exist yet ...

@cwrau
Copy link

cwrau commented Mar 25, 2024

Is there currently a way?

@cwrau A couple of things here. Firstly, it looks like the error you got was because the experiment wasn't enabled. If I use TASK_X_ANY_VARIABLES=1 task test with your example, I get a different error:

template: :1:15: executing "" at <.tool.url>: can't evaluate field url in type interface {}

I guess you're right, sorry 😅

This error occurs because the field .url does not exist on the variable .tool because it is a string, not a map.

When you do:

vars:
  tool: '{{ .tools.controller_gen }}'

This is not creating a variable of type map[string]any as you're expecting. As I said previously, all templates output strings, so Go is stringifying the map into something that looks like this: "map[url:sigs.k8s.io/controller-tools/cmd/controller-gen version:0.14.0]".

Ah, I see, I got a bit confused there

With TASK_X_ANY_VARIABLES=1 there is no way to pass variables between tasks without converting them to strings first. This is a shortcoming of the templating system. Your only options would be to use the top-level variables directly:

tasks:
  test:
    cmds:
      - echo '{{ .tools.controller_gen.url }}'

or redefine the bottom-level string values you want:

tasks:
  test:
    vars:
      url: '{{ .tools.controller_gen.url }}'
      version: '{{ .tools.controller_gen.version }}'
    cmds:
      - echo "'{{ .url }}' should be 'sigs.k8s.io/controller-tools/cmd/controller-gen'"
      - echo "'{{ .vesrion }}' should be '0.14.0'"

However, with TASK_X_ANY_VARIABLES=2, you are able to pass variables by ref:

version: '3'

vars:
  tools:
    map:
      controller-gen:
        url: sigs.k8s.io/controller-tools/cmd/controller-gen
        version: 0.14.0

tasks:
  test:
    vars:
      tool:
        ref: tools.controller-gen
    cmds:
      - echo "'{{ .tool.url }}' should be 'sigs.k8s.io/controller-tools/cmd/controller-gen'"

Though, as I paste this example, I've realised that it's not currently working 😆 I will look into this. I imagine the syntax for passing by reference with proposal 1 could look something like this:

version: '3'

vars:
  tools:
    controller-gen:
      url: sigs.k8s.io/controller-tools/cmd/controller-gen
      version: 0.14.0

tasks:
  test:
    vars:
      tool: !tools.controller-gen
    cmds:
      - echo "'{{ .tool.url }}' should be 'sigs.k8s.io/controller-tools/cmd/controller-gen'"

... but this does not exist yet ...

Ou, both examples look interesting, would be amazing to see them.

But for now we'll just use a single, joined field for the full url, that's probably gonna work better with renovate anyways 😅

@pd93 pd93 removed the breaking change Changes that are not backward compatible. label Apr 9, 2024
@pd93
Copy link
Member Author

pd93 commented Apr 9, 2024

We have made the decision in #1547 to merge support for Any Variables in its current state, but without support for maps. Don't worry, map support is still coming! This functionality will now be tracked in #1585 instead.

We have done this because we feel that it may still take some time to add support for maps. We still have many decisions to make regarding the syntax, and depending on these decisions, the required changes may be breaking (which will further delay the feature).

Seeing as support for other variable types (int, bool, array etc) works today without any breaking changes and there has been very little discussion about feedback/changes, we see no reason to delay this functionality and are happy to add this to the next release of v3. We will now be marking this experiment as a candidate and following the next minor release, it will move to released.

@pd93 pd93 added experiment: candidate Experimental feature - Pending final implementation feedback. Minor changes only. and removed experiment: draft Experimental feature - Pending feedback on draft implementation. labels Apr 9, 2024
@task-bot
Copy link
Collaborator

task-bot commented Apr 9, 2024

This experiment has been marked as a candidate! 🔥 This means that the implementation is nearing completion and we are entering a period for final comments and feedback! You can find information about this experiment and how to enable it in our experiments documentation. Please see the experiment workflow documentation for more information on how we release experiments.

@pd93 pd93 moved this from Draft to Candidate in Task Experiments Apr 9, 2024
@pd93 pd93 added experiment: released Experimental feature - Now the default behavior in Task and is no longer an experiment. and removed experiment: candidate Experimental feature - Pending final implementation feedback. Minor changes only. labels May 9, 2024
@pd93 pd93 moved this from Candidate to Released in Task Experiments May 9, 2024
@task-bot
Copy link
Collaborator

task-bot commented May 9, 2024

This experiment has been released! 🚀 This means that it is no longer an experiment and is available in the latest major version of Task. Please see the experiment workflow documentation for more information on how we release experiments.

@task-bot task-bot closed this as completed May 9, 2024
@go-task go-task locked as resolved and limited conversation to collaborators May 9, 2024
@pd93 pd93 added the area: variables Changes related to variables. label May 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: variables Changes related to variables. experiment: released Experimental feature - Now the default behavior in Task and is no longer an experiment.
Projects
Status: Released
Development

No branches or pull requests

6 participants