Don't fail on err in exec (#259)

* Don't fail on err in exec

* Try to make a test

* comment out test

* use fmt.Sprintf, reformat

* rework failure testing

Added ExitCode to the results struct so it's easier to test against. I'm
trying to remove Diagnostics eventually so it should move anyways.

Reworked some failure catching to be more robust for exec.

Tests pass :)

---------

Co-authored-by: Amy Tobey <atobey@equinix.com>

data_for_test.go

@@ -70,7 +70,8 @@ type Results struct {
 	SpanCount     int            // number of spans received
 	EventCount    int            // number of events received
 	TimedOut      bool           // true when test timed out
-	CommandFailed bool           // otel-cli failed / was killed
+	CommandFailed bool           // otel-cli was killed, did not exit() on its own
+	ExitCode      int            // the process exit code returned by otel-cli
 	Span          *tracepb.Span
 	SpanEvents    []*tracepb.Span_Event
 }
@@ -326,8 +327,8 @@ var suites = []FixtureSuite{
 				},
 			},
 			Expect: Results{
-				Config:        otelcli.DefaultConfig(),
-				CommandFailed: true,
+				Config:   otelcli.DefaultConfig(),
+				ExitCode: 1,
 				// strips the date off the log line before comparing to expectation
 				CliOutputRe: regexp.MustCompile(`^\d{4}/\d{2}/\d{2} \d{2}:\d{2}:\d{2} `),
 				CliOutput: "Error while loading environment variables: could not parse OTEL_CLI_VERBOSE value " +
@@ -369,6 +370,7 @@ var suites = []FixtureSuite{
 			},
 		},
 	},
+
 	// regression tests
 	{
 		{
@@ -465,6 +467,23 @@ var suites = []FixtureSuite{
 				},
 			},
 		},
+		{
+			Name: "#258 Commands that exit with a non-zero exit code should report a span",
+			Config: FixtureConfig{
+				CliArgs: []string{"exec",
+					"--endpoint", "{{endpoint}}",
+					"--verbose", "--fail",
+					"--", "false",
+				},
+			},
+			Expect: Results{
+				ExitCode:      1,
+				SpanCount:     1,
+				CliOutput:     "",
+				CommandFailed: false, // otel-cli should exit voluntarily in this case
+				Config:        otelcli.DefaultConfig().WithEndpoint("grpc://{{endpoint}}"),
+			},
+		},
 	},
 	// otel-cli span with no OTLP config should do and print nothing
 	{
@@ -950,17 +969,17 @@ var suites = []FixtureSuite{
 				TestTimeoutMs: 1000,
 			},
 			Expect: Results{
-				CommandFailed: true,
-				CliOutputRe:   regexp.MustCompile(`^\d{4}/\d{2}/\d{2} \d{2}:\d{2}:\d{2} `),
-				CliOutput:     "invalid protocol setting \"xxx\"\n",
-				Config:        otelcli.DefaultConfig().WithEndpoint("{{endpoint}}"),
+				CliOutputRe: regexp.MustCompile(`^\d{4}/\d{2}/\d{2} \d{2}:\d{2}:\d{2} `),
+				CliOutput:   "invalid protocol setting \"xxx\"\n",
+				Config:      otelcli.DefaultConfig().WithEndpoint("{{endpoint}}"),
 				Diagnostics: otelcli.Diagnostics{
 					IsRecording:       false,
 					NumArgs:           7,
 					DetectedLocalhost: true,
 					ParsedTimeoutMs:   1000,
 					Endpoint:          "*",
 					EndpointSource:    "*",
+					ExecExitCode:      1,
 				},
 				SpanCount: 0,
 			},
@@ -1039,10 +1058,10 @@ var suites = []FixtureSuite{
 				},
 			},
 			Expect: Results{
-				CommandFailed: true,
-				CliOutputRe:   regexp.MustCompile(`^\d{4}/\d{2}/\d{2} \d{2}:\d{2}:\d{2} `),
-				CliOutput:     "invalid protocol setting \"roflcopter\"\n",
-				Config:        otelcli.DefaultConfig().WithEndpoint("http://{{endpoint}}"),
+				ExitCode:    1,
+				CliOutputRe: regexp.MustCompile(`^\d{4}/\d{2}/\d{2} \d{2}:\d{2}:\d{2} `),
+				CliOutput:   "invalid protocol setting \"roflcopter\"\n",
+				Config:      otelcli.DefaultConfig().WithEndpoint("http://{{endpoint}}"),
 				Diagnostics: otelcli.Diagnostics{
 					IsRecording:       false,
 					NumArgs:           3,

main_test.go

@@ -167,7 +167,7 @@ func checkAll(t *testing.T, fixture Fixture, results Results) {
 
 	// many of the basic plumbing tests use status so it has its own set of checks
 	// but these shouldn't run for testing the other subcommands
-	if len(fixture.Config.CliArgs) > 0 && fixture.Config.CliArgs[0] == "status" && !fixture.Expect.CommandFailed {
+	if len(fixture.Config.CliArgs) > 0 && fixture.Config.CliArgs[0] == "status" && results.ExitCode == 0 {
 		checkStatusData(t, fixture, results)
 	} else {
 		// checking the text output only makes sense for non-status paths
@@ -485,9 +485,10 @@ func runOtelCli(t *testing.T, fixture Fixture) (string, Results) {
 	t.Logf("[%s] going to exec 'env -i %s %s'", fixture.Name, strings.Join(statusCmd.Env, " "), strings.Join(statusCmd.Args, " "))
 	cliOut, err := statusCmd.CombinedOutput()
 	results.CliOutput = string(cliOut)
+	results.ExitCode = statusCmd.ProcessState.ExitCode()
+	results.CommandFailed = !statusCmd.ProcessState.Exited()
 	if err != nil {
-		results.CommandFailed = true
-		t.Logf("[%s] executing command failed: %s", fixture.Name, err)
+		t.Logf("[%s] command exited with status %d: %s", fixture.Name, results.ExitCode, err)
 	}
 
 	// send stop signals to the timeouts and OTLP server
@@ -497,7 +498,7 @@ func runOtelCli(t *testing.T, fixture Fixture) (string, Results) {
 
 	// only try to parse status json if it was a status command
 	// TODO: support variations on otel-cli where status isn't the first arg
-	if len(args) > 0 && args[0] == "status" && !fixture.Expect.CommandFailed {
+	if len(args) > 0 && args[0] == "status" && results.ExitCode == 0 {
 		err = json.Unmarshal(cliOut, &results)
 		if err != nil {
 			t.Errorf("[%s] parsing otel-cli status output failed: %s", fixture.Name, err)

otelcli/exec.go

@@ -11,6 +11,7 @@ import (
 
 	"github.com/equinix-labs/otel-cli/otlpclient"
 	"github.com/spf13/cobra"
+	tracev1 "go.opentelemetry.io/proto/otlp/trace/v1"
 )
 
 // execCmd sets up the `otel-cli exec` command
@@ -94,7 +95,10 @@ func doExec(cmd *cobra.Command, args []string) {
 	}
 
 	if err := child.Run(); err != nil {
-		config.SoftFail("command failed: %s", err)
+		span.Status = &tracev1.Status{
+			Message: fmt.Sprintf("exec command failed: %s", err),
+			Code:    tracev1.Status_STATUS_CODE_ERROR,
+		}
 	}
 	span.EndTimeUnixNano = uint64(time.Now().UnixNano())