search

mistifyio / go-zfs

Go wrappers for ZFS commands
Apache License 2.0
128 stars 66 forks source link

Properly substitute command path in `Run()` error #90

Closed corhere closed 1 year ago

corhere commented 1 year ago

If the command executed by (*command).Run() exits with a nonzero status, an error is returned which includes the complete command that was executed, including the resolved path to the command binary. Or that's the intention. What it actually did was prepend the resolved path to the command + arguments, chopping off the first byte of the latter. As a result, a call to zfs("list") which would run "/usr/bin/zfs list" would, if the command failed, return an error claiming that "/usr/bin/zfs fs list" was executed! This can mislead users into thinking that the failed command was due to the wrong arguments being passed, when the problem lies elsewhere.

Change (*command).Run() to return an error containing the resolved path and arguments of the failed command without any spurious arguments, and to debug-log the command with the resolved path.

thaJeztah commented 1 year ago

@mmlb @AkihiroSuda @Mic92 PTAL (see https://github.com/moby/moby/issues/45533)

thaJeztah commented 1 year ago

Thanks @nshalman ! Would it be possible to tag a v3.0.1 patch release?

nshalman commented 1 year ago

I've never done a release in this repo before. I'll see if I can get @mmlb to help me do that.

thaJeztah commented 1 year ago

Thank you!

mmlb commented 1 year ago

I can do so in a few a hours when I'm back at a computer.

mmlb commented 1 year ago

Thanks for the PR!

mmlb commented 1 year ago

https://github.com/mistifyio/go-zfs/releases/tag/v3.0.1 is tagged and bagged. fyi @nshalman I just tagged and then ran https://github.com/mistifyio/go-zfs/blob/master/tooling/release.sh (which I just added :D )

nshalman commented 1 year ago

And I'll try to remember that we have mergify configured. Thanks @mmlb!