go-task / task

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

Idea: Consider having functions to access variables and envs #1065

Open andreynering opened 1 year ago

andreynering commented 1 year ago

This is an idea for the next major version, and would be a breaking change.

Nowadays both variables and envs are accessed as Go template values, like this:

{{.MY_VARIABLE}}
{{.MY_ENV}}

This have a couple of drawbacks:

1) Variables need to be declared in an specific order if you want to include one into another:

FOO: foo
FOOBAR: '{{.FOO}}bar'
FOOBARBAZ: '{{.FOOBAR}}baz'

2) Variables and envs are mixed, and this often generate a bit of confusion to users.

3) Shell variables are always computed, even when they are not being used.

This proposal to have two template functions called var and env to access these values:

MY_VARIABLE: '{{var "MY_VARIABLE"}}'
MY_ENV: '{{env "MY_ENV"}}'

As part of these proposal, environment variables would stop being added to variables, so you need to really call env if you want an env.

andreynering commented 1 year ago

To make this clear: this is an idea, but needs experimentation to see if it would really work as expected.

Also, opinion from users are welcome, specially considering that this is a breaking change and would add work to users that will want to upgrade to the next version.

ghostsquad commented 1 year ago

I think this is a great idea. Changing variable access from being a field to a function call enables lazy variables, which solves a big pain point right now.

marco-m-pix4d commented 1 year ago

I agree it would be a good enhancement. It took me a while to understand that {{.FOO}} resolves also env vars and not only Task vars.

Also from a security point of view, it is better not to make env vars resolvable as {{.FOO}} by default. What happened to me is that I wrote something like

tasks:
  hello:
    cmds:
      - 'foo {{.BAR}}'

with BAR being a token coming as env var to Task, thus leaking it to stdout. If BAR were not resolved, then I would have been obliged to write the safer (and arguably easier to understand)

tasks:
  hello:
    cmds:
      - 'foo $BAR'
ameergituser commented 1 year ago

@andreynering what are the problems with:

Variables need to be declared in an specific order if you want to include one into another

This keeps things simple, readable, and maintainable for the user.

ghostsquad commented 1 year ago

@andreynering what are the problems with:

Variables need to be declared in an specific order if you want to include one into another

This keeps things simple, readable, and maintainable for the user.

I too am interested in this answer. I mentioned in a different issue that task uses a map for storing variables, where it really should be using a list/array. Maps inherently don't have an order. Golang even explicitly randomizes map access in loops to discourage any reliance on order. Yet in Task, order of declared keys in variables matters.

andreynering commented 1 year ago

@ghostsquad It's not true that Task store vars in a map. It's stored in an ordered map implementation. That's why we're able to make it work correctly.

ghostsquad commented 1 year ago

@ghostsquad It's not true that Task store vars in a map. It's stored in an ordered map implementation. That's why we're able to make it work correctly.

I'm referring to how it's stored in yaml.

shakfu commented 1 year ago

As a recent newcomer to task, I can confirm that after a few days, I was (and remain) confused about the differences between var and env variables and their interpolation and scoping rules, whether they can be used together, when to use which, and why both are needed...

Concretely, I was translating a part of a Makefile in use to task syntax, and was testing what worked and what didn't:

DEPS = build/deps
VCPKG_ROOT = $(DEPS)/vcpkg
VCPKG = $(VCPKG_ROOT)/vcpkg

to

version: '3'

vars:
  DEPS: build/deps
  VCPKG_ROOT: '{{.DEPS}}/vcpkg'
  VCPKG: '{{.VCPKG_ROOT}}/vcpkg'

env:
  GREETING: Hey, there!
  GREETING2: $GREETING! Friend! 
  # GOODBYE: {{.VCPKG_ROOT}} # this doesn't work: cannot unmarshal !!map into string

tasks:
  default:
    cmds:
      - echo $GREETING
      - echo ${GREETING}
      - echo {{.GREETING}}

      - echo $GREETING2

      - echo $VCPKG_ROOT
      - echo ${VCPKG_ROOT}
      - echo {{.VCPKG_ROOT}}

and then running task:

$ task
task: [default] echo $GREETING
Hey, there!
task: [default] echo ${GREETING}
Hey, there!
task: [default] echo Hey, there!
Hey, there!
task: [default] echo $GREETING2
$GREETING! Friend!
task: [default] echo $VCPKG_ROOT

task: [default] echo ${VCPKG_ROOT}

task: [default] echo build/deps/vcpkg
build/deps/vcpkg

I initially got bitten with following:

So in summary, I agree that these small differences of use and inter-use can be confusing for a newcomer, and even now after a few days with the docs, and some testing behind me, I'm still looking for some further clarity on this subject.

I think a big step would be improving the docs and ensuring that both are discussed together at the same time and to highlight what problem each solves, what are the differences, when to use one vs the other, etc.

With the respect to the solution above, I kind of understand it, but I am slighly confused by the example given to demonstrate its utility:

This proposal to have two template functions called var and env to access these values:

MY_VARIABLE: '{{var "MY_VARIABLE"}}'
MY_ENV: '{{env "MY_ENV"}}'

Does the above mean this would be norm?:

vars:
  a: abc
  b: '{{var "a"}}'

env:
  c: def
  d: '{{env "c"}}'
  e: '{{var "a"}}'

While this would certainly eliminate confusion, it makes the interpolation syntax more verbose than that of Makefiles and regular shell interpolation syntax.

May I suggest better docs, a table of differences, an advisory on usage, as a first step...

marco-m-pix4d commented 1 year ago

@shakfu

May I suggest better docs, a table of differences, an advisory on usage, as a first step...

Would you have time to add a documentation PR to https://github.com/go-task/task/tree/main/docs ?

I am sure that the Task maintainers can point you to a more precise spot under ./docs.

shakfu commented 1 year ago

Hi @marco-m-pix4d

Would you have time to add a documentation PR to https://github.com/go-task/task/tree/main/docs ?

I am sure that the Task maintainers can point you to a more precise spot under ./docs

I'm flattered by the suggestion, but I'm not sure you want someone who has spent only 2-3 days with task to write docs.

As I'm still figuring it out, what I can do, and I do this in any case, is to keep dev notes, and if they do prove to be worth contributing then I'll be more than happy to do so, table of differences and all.

In any case, I think my disconnect stems from my not having any experience with golang templates, so that will be next on the list to learn.

marco-m-pix4d commented 1 year ago

Hello @shakfu

I'm flattered by the suggestion, but I'm not sure you want someone who has spent only 2-3 days with task to write docs.

Actually this is the exact reason why I suggested, because you have fresh eyes for Task, so what you perceive can help newcomers :-)

what I can do, and I do this in any case, is to keep dev notes, and if they do prove to be worth contributing then I'll be more than happy to do so

Very good, thanks!

ghostsquad commented 1 year ago

While this would certainly eliminate confusion, it makes the interpolation syntax more verbose than that of Makefiles and regular shell interpolation syntax.

ya, Go in general is verbose. We need to get past this as a community. This is a feature not a bug.

I agree with @andreynering that implementing lazy vars is the way to go. Introducing a function like:

MY_VARIABLE: '{{var "MY_VARIABLE"}}'
MY_ENV: '{{env "MY_ENV"}}'

is in fact backwards compatible. So we can support both syntaxes, but encourage one over the other in the docs.

Introducing a couple of options (to the vars themselves, not how they are referenced via function) might help:

eager: true (default false) cache: true (default false) though this is not backwards compatible invalidate: true (default false) this is backwards compatible

I could put together a PR for this if no one else is actively doing so.