-
-
Notifications
You must be signed in to change notification settings - Fork 578
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
Comments
Any feedback on this? |
I also just ran into this and was also surprised |
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. |
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. |
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. Nevertheless I would consider documenting (and also adding tests) the current behavior as useful. Shall I put some effort into that? |
@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. |
@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 As I said, I'll post something with much more detail soon. |
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. |
@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 |
The proposal lgtm @pd93 . Nice work. Maybe some additional thoughts:
|
@zbindenren replied to your comments in the proposal discussion to keep everything together. |
I would consider this a bug than a breaking change, honestly ... One of the primary reasons I am moving all the version: '3'
dotenv:
# organization level overwrites .sht.env is reserved
- '{{.HOME}}/.sht.env' |
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. |
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 |
I just ran into this issue because I want to override the 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}} |
@onedr0p the silly workaround for now is to use the
this will let you run
|
Then it is definitely a bug and we should fix it. |
I am also trying to set how and where 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 |
The problem is because of the way variables are implemented in In Take the above coupled with the fact that you have the concept of Having variables that are mutable, as they're implemnted today will be the source of endless bugs. Immutability (like in |
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 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. |
FWIW, this conversation reminds me of Environment variables precedence in Docker Compose |
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
Taskfile-test.ymlversion: '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 |
$ task give:my-hot-take
Action item: Come up with a way to override system environment variables from the taskfile. |
Is there any progress? Can we help somehow? |
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. |
This looks pretty straight forward to me. Any chance we can get this into the next release? |
This may be overly simplifying the problem, but here's my go on a backwards compatible way to accomlpish this. Since (afaik) version: '3'
tasks:
hello-name:
cmd: echo "Hello $NAME"
env:
NAME:
value: "David"
override: true basically just adding Would this work for everyone? In my case I just needed to ensure that the |
I might have missed it but I didn't see it mentioned anywhere that the variable precedence for resolving templates is already well-documented:
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:
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.
|
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. |
As a workaround, this gave the behavior I wanted:
|
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: 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:
Then create a Taskfile.yml testing it. The # 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 |
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
Expected
I would expect the environment variable of KEY to be overwritten whatever the outside environment looks like.
Actual
The environment variable from the outside shell takes precedence over the variable defined within the taskfile.
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?
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.The text was updated successfully, but these errors were encountered: