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

(bug) Params and Env do not produce identical outputs #601

Closed
zph opened this issue Jul 5, 2024 · 1 comment · Fixed by #602
Closed

(bug) Params and Env do not produce identical outputs #601

zph opened this issue Jul 5, 2024 · 1 comment · Fixed by #602

Comments

@zph
Copy link
Contributor

zph commented Jul 5, 2024

Expectation: using params or env vars would both inject environmental variables into any subprocesses executed in the DAG.

Actual: they both inject env vars but differ in how they treat quotes.

Test dag:

params: TEST_PARAM="something" TEST_PARAM2="SOMETHING ELSE"
env:
- ENV_PARAM: "something"
steps:
  - name: step1
    command: bash $HOME/src/dagu/tester.sh
#!/usr/bin/env bash
# tester.sh

printf "TEST_PARAM is %s | TEST_PARAM2 is %s\n" "$TEST_PARAM" "$TEST_PARAM2"

echo $TEST_PARAM/and/path
echo $ENV_PARAM/and/path

Results in output:

TEST_PARAM is "something" | TEST_PARAM2 is "SOMETHING ELSE"
"something"/and/path
something/and/path

Which is interpreted subtly differently in the inner script and caused me to debug and workaround it for a dag.

What I expect as output is:

TEST_PARAM is something | TEST_PARAM2 is SOMETHING ELSE
something/and/path
something/and/path

Note the difference for TEST_PARAM and ENV_PARAM. We can't leave off the quote marks for params, otherwise the arguments are parsed incorrectly by splitting on whitespace.

Because some users could rely on this behavior, one solution is to enable params key to take more structured data, like env: key.

Like this:

params: 
- TEST_PARAM: "something" 
- TEST_PARAM2: "SOMETHING ELSE"
env:
- ENV_PARAM: "something"
steps:
  - name: step1
    command: bash $HOME/src/dagu/tester.sh

If taking this approach, the way I think it can keep backwards compatibility is to support both single string and structured map as arguments to params for a period of versions.

Before I dig into where this is happening and offer a code fix:

  1. Does this sound like an issue to you or is there something I'm doing wrong with using dagu?
  2. Do you have preferences on the solution before I start prototyping?

Thanks @yohamta !

@zph
Copy link
Contributor Author

zph commented Jul 5, 2024

Debugging:

I see stringifyParam() intentionally wraps them back in " and then sets them via os.Setenv (https://github.com/dagu-dev/dagu/blob/main/internal/dag/builder.go#L457-L473).

Here's the wrapping in quotes: https://github.com/dagu-dev/dagu/blob/main/internal/dag/builder.go#L428C6-L435

Then added on as the dag.Env: https://github.com/dagu-dev/dagu/blob/main/internal/dag/builder.go#L297-L300

At least for injecting into env, I don't think it's necessary to wrap in quotes here: https://github.com/dagu-dev/dagu/blob/main/internal/dag/builder.go#L430. If there's no other behavior depending on this, it's an easy fix ❤️ .

Ok, I'll put up a PR for that, and see if it passes tests. If so, I'll add a test specifically for this case.

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 a pull request may close this issue.

1 participant