Skip to content

Commit

Permalink
(bug) Fix params quoting when using quotes on value (#602)
Browse files Browse the repository at this point in the history
* (bug) Fix params quoting when using quotes on value

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
```

* Adjust tests to reflect there's no forced quoting
  • Loading branch information
zph committed Jul 5, 2024
1 parent fdcc93a commit 9ef8153
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 11 deletions.
7 changes: 2 additions & 5 deletions internal/dag/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -426,13 +426,10 @@ func parseParams(

// stringifyParam converts a paramPair to a string representation.
func stringifyParam(param paramPair) string {
escapedValue := strings.ReplaceAll(param.value, `"`, `\"`)
quotedValue := fmt.Sprintf(`"%s"`, escapedValue)

if param.name != "" {
return fmt.Sprintf("%s=%s", param.name, quotedValue)
return fmt.Sprintf("%s=%s", param.name, param.value)
}
return quotedValue
return param.value
}

// processParams parses and processes the parameters for the DAG.
Expand Down
12 changes: 6 additions & 6 deletions internal/dag/builder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -156,12 +156,12 @@ func TestBuilder_BuildParams(t *testing.T) {
"Y": "foo",
"Z": "A B C",
"1": "first",
"2": `P1="foo"`,
"3": `P2="BAR"`,
"4": `P3="BAR"`,
"5": `X="bar"`,
"6": `Y="foo"`,
"7": `Z="A B C"`,
"2": `P1=foo`,
"3": `P2=BAR`,
"4": `P3=BAR`,
"5": `X=bar`,
"6": `Y=foo`,
"7": `Z=A B C`,
},
},
}
Expand Down

0 comments on commit 9ef8153

Please sign in to comment.