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

Environment variables from shell takes precedence over task file definition #1038

Open
jaedle opened this issue Mar 7, 2023 · 33 comments
Open
Labels
breaking change Changes that are not backward compatible. experiment: draft Experimental feature - Pending feedback on draft implementation.

Comments

@jaedle
Copy link
Contributor

jaedle commented Mar 7, 2023

Hey 👋

I guess I have found a bug yesterday on task: It looks like previously defined environment variable are not overwritten by the definition within the task file.

Taskfile

version: '3'

silent: true

tasks:
  default:
    env:
      KEY: 'other'
    cmds:
      - echo "$KEY"

Expected

I would expect the environment variable of KEY to be overwritten whatever the outside environment looks like.

$ task
other
$ KEY=some task
other

Actual

The environment variable from the outside shell takes precedence over the variable defined within the taskfile.

$ task
other
$ KEY=some task
some

Os + Taskfile-version

$ cat /etc/lsb-release 
DISTRIB_ID=LinuxMint
DISTRIB_RELEASE=21.1
DISTRIB_CODENAME=vera
DISTRIB_DESCRIPTION="Linux Mint 21.1 Vera"
$ task --version
Task version: v3.21.0 (h1:pVGAGXxJ9Pk5mvjqv/CsdAD4ZIzNMjF+vrXMueM/WFk=)

Why is that problem?

  1. I have not found any reference to that behavior in the docs.
  2. I love to clearly define my environment when using task rather than relying on any other outer state. In that case it's impossible to overwrite previously defined variables which may be a huge issue.
@jaedle
Copy link
Contributor Author

jaedle commented May 1, 2023

Any feedback on this?

@matthchr
Copy link

matthchr commented May 3, 2023

I also just ran into this and was also surprised

@andreynering andreynering added breaking change Changes that are not backward compatible. and removed state: needs triage Waiting to be triaged by a maintainer. labels May 23, 2023
@andreynering
Copy link
Member

Unfortunately, this is a breaking change. This means we'd have to wait for the next major version release to do this by default, and that can take quite some time to happen.

In the meantime, we can consider having an opt-in setting for the behavior, so those who want to anticipate that can just set a flag.

@zbindenren
Copy link
Contributor

In my opinion this is a bug. If I set a variable in the command I expect it to run with this configuration. Otherwise the stuff I see in the Taskfile is not what is happening when running the task. This is really confusing for the user.

If you are not willing to change the behavior because of backward compatibility concerns I would prefer to set the opt-in option in the Taskfile, otherwise when I forget it, commands are not working.

@jaedle
Copy link
Contributor Author

jaedle commented May 23, 2023

Thanks for your feedback!

I kindly agree with @zbindenren considering it a bug.

I also agree that this may be a breaking chance in respect of current behavior even though it was never documented.

I would argue that my pull request is not relevant anymore in respect of the direction you are suggesting.
Shall I close it?

Nevertheless I would consider documenting (and also adding tests) the current behavior as useful.
When creating the PR changing the behavior it did not break any test.

Shall I put some effort into that?

@andreynering
Copy link
Member

andreynering commented May 23, 2023

@jaedle I'm sure that some people like and use the current behavior. So, if you're willing to work on it, I think we should have a setting on the Taskfile to opt-in the new behavior. Something like:

disable_os_env_precedence: true

(Consider this name WIP. Naming things is hard. Suggestions are welcome. 🙂)

/cc @pd93. I think you had some ideas on how to deal with breaking changes and flags like this. This may be an opportunity to start discussing it.

@pd93
Copy link
Member

pd93 commented May 23, 2023

@andreynering yes, I've actually started writing up a proposal for breaking changes, but didn't have much time to work on it last week. I'll see if I can get it sorted in the next day or two and I'll link it back to here when I've posted it.

Very quick thoughts:

I'm not a huge fan of adding experimental flags to the Taskfile itself. I think it can muddy the schema a lot - especially if the behaviour may change or be removed entirely in a future major release. However, if we're proposing a permanent change (i.e. users will be able to make this choice going forwards into new major versions), then I don't consider this to be a breaking change at all (as long as the default behaviour stays the same). We can always have a discussion about changing the default behaviour at a later date.

Since this is a hotly debated topic, it might be nice to put this behind an experimental flag before making it a permanent feature of the Taskfile so that users can try it out. That way, we're not making more breaking changes in the future if it's not implemented to everyone's liking. This kind of flag would be enabled with a TASK_X_ENV_PRECEDENCE envvar. This allows users to try out the behaviour with one-off commands (via flag) or by setting up their environment (envvar).

As I said, I'll post something with much more detail soon.

@jaedle
Copy link
Contributor Author

jaedle commented May 26, 2023

Thanks for your feedback! This is really appreciated.

As we are not sure about the direction up l would feel free to document the current behaviour in tests + docs, if you are fine with it.

@pd93
Copy link
Member

pd93 commented May 27, 2023

@andreynering @jaedle @zbindenren Apologies for the delay, I've been travelling for work a lot lately, but I had some time today to write up the breaking changes proposal. Comments/thoughts are very welcome

@zbindenren
Copy link
Contributor

zbindenren commented May 30, 2023

The proposal lgtm @pd93 . Nice work. Maybe some additional thoughts:

  • how long is a flag experimental? Maybe we should do it like kubernetes does. 3-6 months and then it is dropped or added.
  • how do we introduce breaking changes to the taskfile yaml itself. Maybe define when and how and under what circumstances to relaease a v4 etc...

@pd93
Copy link
Member

pd93 commented May 30, 2023

Maybe some additional thoughts:

@zbindenren replied to your comments in the proposal discussion to keep everything together.

@pd93 pd93 added the experiment: proposed Experimental feature - Pending feedback on proposal. label May 30, 2023
@pd93 pd93 moved this to Todo in Task Experiments Jun 15, 2023
@yordis
Copy link

yordis commented Oct 30, 2023

I would consider this a bug than a breaking change, honestly ...

One of the primary reasons I am moving all the Makefile and scripts to Taskfile is that I can control which .env files I am loading, and hopefully ignore whatever the programmer decided to do for the personal preferences:

version: '3'

dotenv:
  # organization level overwrites .sht.env is reserved
  - '{{.HOME}}/.sht.env'

@yordis
Copy link

yordis commented Oct 30, 2023

Kind of related to this, #1384

I couldn't find documentation about the order of loading these env variables ... reading the code something told me I was expecting something that isn't possible.
Would be nice to, at the very least, start with documenting the existing order (regardless if it is "broken" or not).

@carmilso
Copy link

I also find this as a bug. Expected behavior would be to overwrite environment variables, not respect them. So for example if I want to define a global .dotenv file where I redefine the PATH, why can't I do that?

@onedr0p
Copy link
Contributor

onedr0p commented Nov 27, 2023

I just ran into this issue because I want to override the KUBECONFIG env var on a task, here's a snippet of what I would like to be able to do. However since KUBECONFIG is exported in my shell it won't get overridden. I'll have to discover another way to handle this in the meantime.

version: "3"
tasks:
  sync:
    desc: Sync ExternalSecret resources
    summary: task {{.TASK}} [cluster=main] [ns=default]? [secret=plex]?
    cmd: |
      {{if eq .secret ""}}
        kubectl get externalsecret.external-secrets.io --all-namespaces --no-headers -A | awk '{print $1, $2}' \
          | xargs --max-procs=4 -l bash -c 'kubectl -n $0 annotate externalsecret.external-secrets.io $1 force-sync=$(date +%s) --overwrite'
      {{else}}
        kubectl -n {{.ns}} annotate externalsecret.external-secrets.io {{.secret}} force-sync=$(date +%s) --overwrite
      {{end}}
    env:
      KUBECONFIG: "{{.ROOT_DIR}}/kubernetes/{{.cluster}}/kubeconfig"
    vars:
      ns: '{{.ns | default "default"}}'
      secret: '{{ .secret | default ""}}'
    preconditions:
      - kubectl -n {{.ns}} get externalsecret {{.secret}}

@clintmod
Copy link
Contributor

clintmod commented Nov 29, 2023

@onedr0p the silly workaround for now is to use the default template function on a var and then assign it to the env like this:

# yaml-language-server: $schema=https://taskfile.dev/schema.json
version: '3'

tasks:
    
    default:
      vars:
        KUBECONFIG: '{{ default "/your/default/kubeconfig/path/here" .KUBECONFIG }}'
      env:
        KUBECONFIG: '{{ .KUBECONFIG }}'
      cmds:
        - echo {{.KUBECONFIG}}

this will let you run

23-11-28 20:35:08|~/Desktop|main|+110-95|[!?]|15ms|󰈺 ❯ task
task: [default] echo /your/default/kubeconfig/path/here
/your/default/kubeconfig/path/here

23-11-28 20:36:31|~/Desktop|main|+110-95|[!?]|13ms|󰈺 ❯ KUBECONFIG=ASDF task
task: [default] echo ASDF
ASDF

@zbindenren
Copy link
Contributor

Then it is definitely a bug and we should fix it.

@yasn77
Copy link

yasn77 commented Dec 6, 2023

I am also trying to set how and where KUBECONFIG is aassigned and, following @clintmod suggestion, found some odd behaviour. When including a Taskfile, it doesn't seem like the vars can be interpolated under the includes section. Here is my example:

Taskfile.yml

# https://taskfile.dev

version: '3'

vars:
  KUBECONFIG: '/path/to/test1/kubeconfig.yaml'
  KUBECONFIG_TEST4: '/path/to/test4/kubeconfig.yaml'

includes:
  include-test1:
    taskfile: https://gist.githubusercontent.com/yasn77/85a22ca5fe3fde62247a8a10e8e72310/raw/Taskfile-kubeconfig-test.yml
    vars:
      KUBECONFIG: '{{.KUBECONFIG}}'
  include-test2:
    taskfile: https://gist.githubusercontent.com/yasn77/85a22ca5fe3fde62247a8a10e8e72310/raw/Taskfile-kubeconfig-test.yml
    vars:
      KUBECONFIG: '/path/to/test2/kubeconfig.yaml'

tasks:

  default:
    desc: "default"
    cmds:
      - task: include-test1:get-kubeconfig
      - task: include-test2:get-kubeconfig
      - task: include-test1:get-kubeconfig
        vars:
          KUBECONFIG: '/path/to/test3/kubeconfig.yaml'
      - task: include-test1:get-kubeconfig
        vars:
          KUBECONFIG: '{{.KUBECONFIG_TEST4}}'

Included Gist

# https://taskfile.dev

version: '3'

vars:
  KUBECONFIG: "/path/to/incuded/kubeconfig.yaml"

tasks:
  get-kubeconfig:
    desc: "Show Kubeconfig location"
    env:
      KUBECONFIG: "{{.KUBECONFIG}}"
    cmds:
      - echo "KUBECONFIG Location - $KUBECONFIG" 

Expected Output

➜ task -s
KUBECONFIG Location - /path/to/test1/kubeconfig.yaml
KUBECONFIG Location - /path/to/test2/kubeconfig.yaml
KUBECONFIG Location - /path/to/test3/kubeconfig.yaml
KUBECONFIG Location - /path/to/test4/kubeconfig.yaml

Actual Output

➜ task -s
KUBECONFIG Location - /path/to/incuded/kubeconfig.yaml
KUBECONFIG Location - /path/to/test2/kubeconfig.yaml
KUBECONFIG Location - /path/to/test3/kubeconfig.yaml
KUBECONFIG Location - /path/to/test4/kubeconfig.yaml

Test 1 is not correctly interpolating the variable 😞

If it's deemed needed, I am happy to open a new issue about this

@clintmod
Copy link
Contributor

clintmod commented Dec 12, 2023

The problem is because of the way variables are implemented in go-task. They're backwards in how they function in a shell and in other build systems. In other build systems they're immutable and designed to be overriden from the top down (first in wins).

In go-task it is bottom up.

Take the above coupled with the fact that you have the concept of vars vs env vars and you get this unexpected co-mingled behavior for something that you only interact with from the command line and do not expect there to be a separate concept between env vars and vars.

Having variables that are mutable, as they're implemnted today will be the source of endless bugs.

Immutability (like in make and ant) is much more predictable and easier to reason about.

@krystian-panek-vmltech
Copy link

krystian-panek-vmltech commented Jan 15, 2024

I very often organize automation in task files but if some variable is already defined on the OS level there is no way to override it in any of the env sections in taskfile. the last resort is to export/override vars in cmd but this then needs to be copied/pasted to all commands that may leverage that overridden env var which may be ugly to do so and lead to errors.

see my command describing it briefly: #993 (comment)

I like GoTask but this issue is coming back to me over and over again. It would be nice to address mine and other people's problems someday.

@ericslandry
Copy link

FWIW, this conversation reminds me of Environment variables precedence in Docker Compose

@liiight
Copy link

liiight commented Mar 21, 2024

I'm not sure if this falls to the same source issue but it seems there is an issue with dotenv precedence as well:

.env.test

FOO=default

Taskfile-test.yml

version: '3'
dotenv:
  - .env.test
tasks:
  default:
    cmds:
      - echo "{{.FOO}}"
      - echo "$FOO"

Output

❯ task -t Taskfile-test.yml
task: [default] echo "default"
default
task: [default] echo "$FOO"
default
❯ FOO=not-default task -t Taskfile-test.yml
task: [default] echo "default"
default
task: [default] echo "$FOO"
not-default

If this is an unrelated issue i'd be happy to open a new one

@jwater7
Copy link
Contributor

jwater7 commented May 29, 2024

$ task give:my-hot-take

  • I can see why someone could observe that there's "no correct way of doing this". But I disagree with conclusion that there should be nothing done because of that. The main complaint by everyone is simply that we have no good way of overriding an environment variable (whether using variable precedence or otherwise).
  • I don't agree that this is a bug because it's clearly documented how it should work - however, I don't believe the behavior is implemented in the most useful way. Ideally, a breaking change should be made to reverse the precendence because I think the the "democratic majority" of people expect variables in a taskfile to override ones provided by their system - I can't think of another system that does it the way taskfile does currently, all other yamls that we are used to like docker-compose, k8s, gitlab all override. This behavior is essentially going to be a never-ending issue producer. So at the very least, a method how to override environment variables within the taskfile should also be documented along with the precedence docs.

Action item: Come up with a way to override system environment variables from the taskfile.

Related issues:
#1272 #993 #1276 #482

@eyalch
Copy link

eyalch commented Jun 16, 2024

Is there any progress? Can we help somehow?

@eccles
Copy link

eccles commented Jun 17, 2024

Hi everyone - came to this discussion because I have the same issue. My solution was to 'source' a file that defines PATH and other env vars in every invocation of cmd:. Obviously this is not ideal but I would suggest adding an option 'sourcefile' which if specified will source the file viz:

sourcefile: "{{.TOOLS_SOURCE}}"
tasks:
   environment:
    desc: Show environment
    cmds:
     -|
       env | sort

I use this to ensure that specific b=versions of tools such as the golang compiler are used and the sourcing of the file prepends the paths to these specific versions to the PATH variable.

@mgbowman
Copy link
Contributor

mgbowman commented Jul 1, 2024

This looks pretty straight forward to me. Any chance we can get this into the next release?

@davidbarratt
Copy link

This may be overly simplifying the problem, but here's my go on a backwards compatible way to accomlpish this. Since (afaik) env accepts a Variable object:

version: '3'
tasks:
  hello-name:
    cmd: echo "Hello $NAME"
    env:
      NAME:
        value: "David"
        override: true

basically just adding value which can be used if sh is not used and override which overrides whatever value is already set.

Would this work for everyone? In my case I just needed to ensure that the PORT variable is not overridden. Because of this bug, it sets every service to the same port! 😞

@nathanperkins
Copy link

nathanperkins commented Aug 10, 2024

I might have missed it but I didn't see it mentioned anywhere that the variable precedence for resolving templates is already well-documented:

Variables can be set in many places in a Taskfile. When executing templates, Task will look for variables in the order listed below (most important first):

  • Variables declared in the task definition
  • Variables given while calling a task from another (See Calling another task above)
  • Variables of the included Taskfile (when the task is included)
  • Variables of the inclusion of the Taskfile (when the task is included)
  • Global variables (those declared in the vars: option in the Taskfile)
  • Environment variables

Having different (undocumented?) precedence for env vars resolved in commands is very surprising. And as others have noted, the current precedence makes it impossible to avoid leaking the env into your automation tasks.

Another surprising thing that doesn't work the way I'd expect:

env:
  VAR: foo

tasks:
  bar:
    cmd: VAR={{.VAR}} echo ${VAR}

I'd expect to see foo in both places, but I still see the env var from the execution environment instead of the one defined in taskfile.

$ export VAR=baz

$ task bar
task: [bar] VAR=foo echo ${VAR}
baz

@nathanperkins
Copy link

nathanperkins commented Aug 10, 2024

@pd93

I'm not a huge fan of adding experimental flags to the Taskfile itself.

I hope we can reconsider on this one.

We are using env vars to set the KUBECONFIG to a temporary file so that we know it is connected to the correct cluster and so that we don't interfere with the context used by the user's CLI. Using the correct env vars in this taskfile is critical to ensure that people don't accidentally deploy workloads to the wrong environment. It would be dangerous to rely on everybody configuring the correct experimental value.

@nathanperkins
Copy link

As a workaround, this gave the behavior I wanted:

vars:
  VAR: foo

tasks:
  bar:
    cmd: |
      export VAR={{.VAR}}
      echo ${VAR}

@gedw99
Copy link

gedw99 commented Aug 15, 2024

For anyone arriving here, the fix is in main, and it works perfectly for me:

Huge thank to @vmaerten and @andreynering

I am on darwin. Have not tested on windows or linux yet. So update here if this works for you.

The fix is here if your curious:
4b6c79a

Make sure you get main:

go install github.com/go-task/task/v3/cmd/task@main

Then create .env in the same folder your testing this, and turn on the experiment like so:

# task experiments
# task automatically uses this .env file.
# https://taskfile.dev/experiments/

#TASK_X_GENTLE_FORCE=1
#TASK_X_MAP_VARIABLES=1
#TASK_X_REMOTE_TASKFILES=1
TASK_X_ENV_PRECEDENCE=1

Then create a Taskfile.yml testing it. The task run-test just calls binaryname in the .dep or .bin folder

# yaml-language-server: $schema=https://taskfile.dev/schema.json

version: '3'

set: [pipefail]

run: when_changed

env:
  BIN_ROOT: "{{.USER_WORKING_DIR}}/.bin"
  DEP_ROOT: "{{.USER_WORKING_DIR}}/.dep"
  PATH: "{{.BIN_ROOT}}:{{.DEP_ROOT}}:{{.PATH}}"
vars:
  REPO_ROOT: "{{.ROOT_DIR}}"
  BUILD_ROOT: "{{.ROOT_DIR}}/build"
  DOCKER: '{{.DOCKER | default "docker"}}'

tasks:
  print:
    desc: Print variables 
    cmds:
      - echo "print"
      - echo ""
      - echo "REPO_ROOT" $REPO_ROOT
      - echo "BIN_ROOT" $BIN_ROOT
      - echo "DEP_ROOT" $DEP_ROOT
      - echo ""
      - echo "PATH" $PATH
  run-test:
    desc: Run tests 
    cmds:
      - echo "test"
      - binaryname {{.CLI_ARGS}}

Then create the sub folders:

mkdir -p $PWD/.bin
mkdir -p $PWD/.dep

Then drop a binary into one of sub folders:

cp <<some binary>> $PWD/.dep/binaryname

Then run it:

task run-test

@pd93 pd93 added experiment: draft Experimental feature - Pending feedback on draft implementation. and removed experiment: proposed Experimental feature - Pending feedback on proposal. labels Sep 9, 2024
@task-bot
Copy link
Collaborator

task-bot commented Sep 9, 2024

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.

@dennisconrad
Copy link

Expanding on @davidbarratt's #1038 (comment):

Currently, this behaves like make's FOO?="BAR":

vars:
  FOO: BAR

A backwards compatible way would be to keep vars: as is, and add a way to emulate make's FOO:="BAR" like this or with another more appropriate section name:

overrides:
  FOO: BAR

(or @davidbarratt's more verbose idea). This would allow task to emulate both of make's FOO?="BAR" and FOO:="BAR". This is especially important if you need to temporarily modify the PATH in a Taskfile, for example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Changes that are not backward compatible. experiment: draft Experimental feature - Pending feedback on draft implementation.
Projects
Status: Draft