Skip to content

exec passes trailing arguments to 'sh -c' #191

@garthk

Description

@garthk

G'day! I'd expected otel-cli exec to invoke its first argument with its remaining arguments, so the first result below surprised me until I dug into the source code and saw it matched the second:

$ otel-cli exec -- bash -c 'echo a; echo b'

b
$ sh -c 'bash -c echo a; echo b'

b

I'd hoped for stricter argument passing so I could put otel-cli anywhere without having to troubleshoot whitespace and escaping in arguments, eg. so I could safely put otel-cli in a shebang to wrap a script:

#!/usr/bin/env otel-cli exec -- python3
import os, sys
print("Arguments:", repr(sys.argv))
print("Traceparent:", os.environ["TRACEPARENT"])

Propagation works, but with the current argument handling the argument gets split on its whitespace:

$ ./script.py "hello there"
Arguments: ['./script.py', 'hello', 'there']
Traceparent: 00-e16154a608fa3a412d3f8f4739a950a2-ea907477c0ba37b7-01

Straight argument passing would be a breaking change demanding a minor version bump given your major is zero… I reckon at about the same scale as #181? A lot of people won't notice, but it's worth documenting.

Proof-of-concept patch:

The following fixes the problem for POSIX-flavoured systems only:

diff --git otelcli/exec.go otelcli/exec.go
index 46df894..31560c1 100644
--- otelcli/exec.go
+++ otelcli/exec.go
@@ -52,13 +52,10 @@ func doExec(cmd *cobra.Command, args []string) {
 
-       // use cmd.exe to launch PowerShell on Windows
-       var shell string
-       if runtime.GOOS == "windows" {
-               shell = "cmd.exe"
+       var child *exec.Cmd
+       if runtime.GOOS == "windows" { // use cmd.exe to launch PowerShell on Windows
                commandString = "/C powershell " + commandString
+               child = exec.Command("cmd.exe", "-c", commandString)
        } else {
-               shell = "/bin/sh"
+               child = exec.Command(args[0], args[1:]...)
        }
 
-       child := exec.Command(shell, "-c", commandString)
-
        // attach all stdio to the parent's handles

A full PR might remove the special case for Windows, leaving that to the Go standard library (syscall.StartProcess?).

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingdocumentationImprovements or additions to documentationminorminor semver

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions