Skip to content

Commit

Permalink
Revert set -x functionality.
Browse files Browse the repository at this point in the history
  • Loading branch information
@sirikon
sirikon committed Dec 24, 2024
1 parent edaac63 commit 9f9d186
Show file tree
Hide file tree
Showing 5 changed files with 10 additions and 45 deletions.
11 changes: 1 addition & 10 deletions docs/README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ Finally, it will execute the plan, running tasks sequentially until the end.

<details markdown="1">
<summary>
All scripts include <code>set -euo pipefail</code>. <code>script</code> includes <code>set -x</code>.
All scripts include <code>set -euo pipefail</code>.
</summary>

Before running any Bash script in `script`, `when.output_changes` or `when.check_fails`, Ebro will prepend to the script the lines `set -euo pipefail` to ensure sane defaults:
Expand All @@ -79,10 +79,6 @@ Before running any Bash script in `script`, `when.output_changes` or `when.check
- `-u`: Usage of unset variables is considered an error
- `-o pipefail`: The pipeline’s return status is the value of the last (rightmost) command to exit with a non-zero status, or zero if all commands exit successfully

In case of `script`, it will also add `set -x`:

- `-x`: Print a trace of simple commands.

More on Bash's documentation: [The Set Builtin](https://www.gnu.org/software/bash/manual/bash.html#The-Set-Builtin).

</details>
Expand All @@ -91,15 +87,10 @@ During the first execution it will execute everything, with no skips, which soul

```
███ [:cache_dir] running
+ mkdir -p cache
███ [:produce_a] running
+ echo 'this is A'
███ [:produce_b] running
+ echo 'this is B'
███ [:echoer] running
+ cat cache/A.txt
this is A
+ cat cache/B.txt
this is B
███ [:producer] satisfied
███ [:default] satisfied
Expand Down
5 changes: 5 additions & 0 deletions docs/changelog/0.3.0.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
## Breaking changes

### `set -x` added in previous version have been reverted

It's more secure to let users call themselves `set -x` whenever they need it instead of enabling it by default. It's easy to leak secrets on stdout with it enabled.
11 changes: 4 additions & 7 deletions src/internal/runner/runner.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func Run(inv inventory.Inventory, plan planner.Plan, force bool) error {
if task.When.CheckFails != "" {
output := bytes.Buffer{}
outputWriter := bufio.NewWriter(&output)
status, err := runScriptWithIO(task.When.CheckFails, task.WorkingDirectory, task.Environment, outputWriter, outputWriter, false)
status, err := runScriptWithIO(task.When.CheckFails, task.WorkingDirectory, task.Environment, outputWriter, outputWriter)
if err != nil {
return fmt.Errorf("running task %v when.check_fails: %w", taskName, err)
}
Expand All @@ -48,7 +48,7 @@ func Run(inv inventory.Inventory, plan planner.Plan, force bool) error {
if task.When.OutputChanges != "" {
output := bytes.Buffer{}
outputWriter := bufio.NewWriter(&output)
status, err := runScriptWithIO(task.When.OutputChanges, task.WorkingDirectory, task.Environment, outputWriter, outputWriter, false)
status, err := runScriptWithIO(task.When.OutputChanges, task.WorkingDirectory, task.Environment, outputWriter, outputWriter)
if err != nil {
return fmt.Errorf("running task %v when.output_changes: %w", taskName, err)
}
Expand Down Expand Up @@ -101,14 +101,11 @@ func logLine(taskName string, message string) string {
}

func runScript(script string, workingDirectory string, environment map[string]string) (uint8, error) {
return runScriptWithIO(script, workingDirectory, environment, os.Stdout, os.Stdout, true)
return runScriptWithIO(script, workingDirectory, environment, os.Stdout, os.Stdout)
}

func runScriptWithIO(script string, workingDirectory string, environment map[string]string, stdout io.Writer, stderr io.Writer, simpleCommandTracing bool) (uint8, error) {
func runScriptWithIO(script string, workingDirectory string, environment map[string]string, stdout io.Writer, stderr io.Writer) (uint8, error) {
script_header := []string{"set -euo pipefail"}
if simpleCommandTracing {
script_header = append(script_header, "set -x")
}

file, err := syntax.NewParser().Parse(strings.NewReader(strings.Join(script_header, "\n")+"\n"+script), "")
if err != nil {
Expand Down
6 changes: 0 additions & 6 deletions tests/src/git_import_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,13 @@ def test_git_import_works(self, repository_url):
f"""
███ cloning {repository_url}
███ [:apt:pre-config] running
+ mkdir -p {self.workdir}/.cache/apt/packages
███ [:caddy:package-apt-config] running
+ echo caddy
███ [:apt:default] running
+ echo 'Installing apt packages'
Installing apt packages
+ cat {self.workdir}/.cache/apt/packages/caddy.txt
caddy
███ [:caddy:package] satisfied
███ [:caddy:default] satisfied
███ [:default] running
+ echo Done!
Done!
""",
)
Expand All @@ -49,7 +44,6 @@ def test_git_import_works(self, repository_url):
███ [:caddy:package] satisfied
███ [:caddy:default] satisfied
███ [:default] running
+ echo Done!
Done!
""",
)
Expand Down
22 changes: 0 additions & 22 deletions tests/src/run_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,23 +10,17 @@ def test_execution_is_correct(self):
stdout,
f"""
███ [:apt:pre-config] running
+ mkdir -p {self.workdir}/.cache/apt/packages
███ [:caddy:package-apt-config] running
+ echo caddy
███ [:docker:package-apt-config] running
+ echo 'docker==2.0.0-1-apt'
███ [:apt:default] running
+ echo 'Installing apt packages'
Installing apt packages
+ cat {self.workdir}/.cache/apt/packages/caddy.txt {self.workdir}/.cache/apt/packages/docker.txt
caddy
docker==2.0.0-1-apt
███ [:caddy:package] satisfied
███ [:docker:package] satisfied
███ [:caddy:default] satisfied
███ [:docker:default] satisfied
███ [:default] running
+ echo Done!
Done!
""",
)
Expand All @@ -46,7 +40,6 @@ def test_execution_is_correct(self):
███ [:caddy:default] satisfied
███ [:docker:default] satisfied
███ [:default] running
+ echo Done!
Done!
""",
)
Expand All @@ -58,23 +51,17 @@ def test_execution_is_correct(self):
stdout,
f"""
███ [:apt:pre-config] running
+ mkdir -p {self.workdir}/.cache/apt/packages
███ [:caddy:package-apt-config] running
+ echo caddy
███ [:docker:package-apt-config] running
+ echo 'docker==2.0.0-1-apt'
███ [:apt:default] running
+ echo 'Installing apt packages'
Installing apt packages
+ cat {self.workdir}/.cache/apt/packages/caddy.txt {self.workdir}/.cache/apt/packages/docker.txt
caddy
docker==2.0.0-1-apt
███ [:caddy:package] satisfied
███ [:docker:package] satisfied
███ [:caddy:default] satisfied
███ [:docker:default] satisfied
███ [:default] running
+ echo Done!
Done!
""",
)
Expand All @@ -86,7 +73,6 @@ def test_scripts_fail_asap(self):
stdout,
f"""
███ [:default] running
+ echo 'This should print'
This should print
UNBOUND_VARIABLE: unbound variable
███ ERROR: task :default returned status code 1
Expand All @@ -100,9 +86,7 @@ def test_failing_scripts_are_not_cached_by_output_changes(self):
stdout,
f"""
███ [:default] running
+ echo 'This should print all the time'
This should print all the time
+ exit 1
███ ERROR: task :default returned status code 1
""",
)
Expand All @@ -113,9 +97,7 @@ def test_failing_scripts_are_not_cached_by_output_changes(self):
stdout,
f"""
███ [:default] running
+ echo 'This should print all the time'
This should print all the time
+ exit 1
███ ERROR: task :default returned status code 1
""",
)
Expand All @@ -127,7 +109,6 @@ def test_required_by_does_not_include_referenced_task_in_plan(self):
stdout,
f"""
███ [:b] running
+ echo B
B
███ [:default] satisfied
""",
Expand All @@ -140,10 +121,8 @@ def test_when_checkers_behave_correctly(self):
stdout,
f"""
███ [:always_fails] running
+ echo Running
Running
███ [:never_fails] running
+ echo Running
Running
███ [:default] satisfied
""",
Expand All @@ -155,7 +134,6 @@ def test_when_checkers_behave_correctly(self):
stdout,
f"""
███ [:always_fails] running
+ echo Running
Running
███ [:never_fails] skipping
███ [:default] satisfied
Expand Down

0 comments on commit 9f9d186

Please sign in to comment.