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) Fix params quoting when using quotes on value #602

Merged
merged 2 commits into from
Jul 5, 2024

Conversation

zph
Copy link
Contributor

@zph zph commented Jul 5, 2024

Fixes issue with yaml key params where it results in different behavior than if the key was passed through env key.

Takes an approach that does not require backwards
incompatible changes nor does it require changing
the schema of yaml files.

Fixes #601

Result with this patch:

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

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

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

Fixes issue with yaml key `params` where it results in different
behavior than if the key was passed through env key.

Takes an approach that does not require backwards
incompatible changes nor does it require changing
the schema of yaml files.

------

**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:
```yaml
params: TEST_PARAM="something" TEST_PARAM2="SOMETHING ELSE"
env:
- ENV_PARAM: "something"
steps:
  - name: step1
    command: bash $HOME/src/dagu/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
```
@zph
Copy link
Contributor Author

zph commented Jul 5, 2024

Questions

Parsing complexity

There's a lot of parsing complexity in builder.go related to params, because it's unstructured. Is it worth introducing the map form and phase out the string form for params?

ie move from

params: TEST_PARAM="something" TEST_PARAM2="SOMETHING ELSE"

to

params: 
- TEST_PARAM: "something"
- TEST_PARAM2: "SOMETHING ELSE"

^^ I don't think the decision needs to be bundled into this PR, but I like that it could remove much of the need for complex parsing around https://github.com/dagu-dev/dagu/blob/main/internal/dag/builder.go#L375-L395

Setenv usage

I'll read through the code but I think it may be possible to avoid the os.Setenv and instead inject those into the command when run. It would avoid the issue of introducing global state.

Copy link
Collaborator

@yohamta yohamta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM🙏✨👍 Thanks a lot for fixing the issue!

@yohamta
Copy link
Collaborator

yohamta commented Jul 5, 2024

@zph Thanks for the excellent suggestion! This sounds great to me. We can accept both structured and unstructured parameters to get the best of both worlds.

params: 
- TEST_PARAM: "something"
- TEST_PARAM2: "SOMETHING ELSE"

@yohamta yohamta merged commit 9ef8153 into dagu-org:main Jul 5, 2024
3 checks passed
@bbqi bbqi mentioned this pull request Jul 27, 2024
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 this pull request may close these issues.

(bug) Params and Env do not produce identical outputs
2 participants