masterzen / winrm

Command-line tool and library for Windows remote command execution in Go
Apache License 2.0
423 stars 129 forks source link

do we need a cmd.Error() function? #41

Open jstrachan opened 8 years ago

jstrachan commented 8 years ago

When running a command inside a Shell like this: https://github.com/fabric8io/kansible/blob/master/winrm/winrm.go#L59-L71

    var cmd *winrm.Command
    cmd, err = shell.Execute(commandText)
    if err != nil {
        return fmt.Errorf("Impossible to create Command %s\n", err)
    }

    go io.Copy(cmd.Stdin, os.Stdin)
    go io.Copy(os.Stdout, cmd.Stdout)
    go io.Copy(os.Stderr, cmd.Stderr)

    cmd.Wait()

I didn't see a way to get the error from the command after the Wait()? I can get the ExitCode though I guess.

hoenirvili commented 8 years ago

Also when I'm running this code it should send somehow a signal on that Command.Done channel? In my example , it just hangs because the empty struct channel waits to recive smth. Does someone tested this enough? @masterzen

masterzen commented 8 years ago

That's true, it seems we're missing an access to the error.

Do you have a test case for failing the error that I could transform in an integration test so I can debug the problem further?

rjhornsby commented 4 years ago

I know this is an old issue, but I think I'm hitting the same problem.

I've been trying to use .Run() like so:

exitcode, err := client.Run("Add-DNSServerResourceRecord -ZoneName myzone.local -A -Name test -IPv4Address 10.0.0.1", os.Stdout, os.Stderr)

But, there's a problem with the PS result. Just for completeness - it doesn't really matter - the error (stderr output) is

'Add-DNSServerResourceRecord' is not recognized as an internal or external command, operable program or batch file.

I get back exitcode 1, but because err is coming back up through the stack as nil, it looks like everything worked.

From my very limited experience with Golang (a few days), it seems like stderr is getting eaten, and spit out to the console as stderr. Because stderr was consumed and wasn't part of the cmd object passed back from Run() as cmd.err, the caller might know that something happened because the exit code was non-zero, but doesn't get any other information. This means that the error message (the error object?) is lost and can't be passed further back up the stack because it's nil.

This really gets tricky when you're calling from tools like Terraform which seem to blackhole stderr (so you never see any error output as a result of io.Copy(stderr, cmd.Stderr)). TF expects to get back an error type if something went wrong. The code I'm working on that interfaces between Terraform and this library doesn't get an error from the call to the library so it passes back what it got - nil, which Terraform takes as if things went well and records the infrastructure change as having been made.

This is an awful solution (pick a reason, I'm sure there are a bunch), but it's what I've come up with so far:

go func() {
    defer wg.Done()
    buf, err := ioutil.ReadAll(cmd.Stderr)
    if err != nil {
        cmd.err = err
    } else {
        if cmd.ExitCode() != 0 {
            cmd.err = fmt.Errorf(string(buf))
        }
    }
}()

At the end of Run(), the stderr handler mentioned above captures stderr, checks the exit code, and if non-zero, sets cmd.err.

I think this the wrong place to handle the error, but it seems like the only place I can capture stderr to be able to pass it back as an error type to the caller.

To be fair, I should also note that I'm not sure how consistent PS is at exiting 0 on success. Windows processes in general seem to exit with whatever the hell code they want, so you get weird crap like 1,35, 259, and 1641 (that's a real example) are all valid exits codes that aren't an indication of an error/failure that should cause the program to uncleanly.