Remove unneeded message on command completion. Set error output to Stderr

[vicmarbev]

Fixes #426 Basically I removed the "Succeeded" message and redirect error output to Stderr. Do you see any problem with this approach? Is there any case that we want to print that "Succeeded" message?

Signed-off-by: Víctor Martínez Bevià [email protected]

[joaopapereira]

Hey @vicmarbev the removal of Succeed text might have some impact on the tools that consume imgpkg as a binary, like vendir or even imgpkg. Is there any particular reason for you to want to remove it?

When you run imgpkg ... | cat you will see that the message does not show up. Would that be enough? another option would be to do imgpkg --tty=false this should also remove messages that are meant for humans to read.

[vicmarbev]

What about that last commit? It's the same approach they used in Kapp about this issue: https://github.com/vmware-tanzu/carvel-kapp/pull/592

[cppforlife]

some work is happening in kapp to clean up similar functionality. will want to port over those changes here. stay tuned.

[cppforlife]

updated kapp change: https://github.com/vmware-tanzu/carvel-kapp/commit/ed8c3f208aaabfac0c678d50cfee120fbd8377d9

[vicmarbev]

Hi @cppforlife could you help me with the IsCobraInternalCommand not declared by package error? I see no difference with the carvel-kapp version

[joaopapereira]

I was looking at the PR and to be fair I am not sure I agree with the name of the function that was added to cobrautil. help is not a cobra internal command. It looks like we are overreaching here with the naming.

[praveenrewar]

I was looking at the PR and to be fair I am not sure I agree with the name of the function that was added to cobrautil. help is not a cobra internal command. It looks like we are overreaching here with the naming.

Yeah, I agree with you on this. help is set by cobra by default but it can also be set manually by us, also it is a little bit tricky in the sense that it's set at runtime unlike other commands. I was trying to describe this behaviour and the best thing I could come up with was internal (as the other commands being checked in that function are cobra internal commands). I am definitely open to changing this up if we can think of a better name.

[joaopapereira]

I think what we want is maybe a function that let us know that we should not provide extra output to them because in some sense this is the only use of this particular function. What do you think of IsCobraManagedCommand?

[praveenrewar]

What do you think of IsCobraManagedCommand?

💯 Makes sense to me.

could you help me with the IsCobraInternalCommand not declared by package error? I see no difference with the carvel-kapp version

@vicmarbev I don't see any diff in the vendor directory, you might wanna run go mod vendor or the ./hack/build.sh script.

[joaopapereira]

I think the problem might be as simple as the vendored packaged was not added to the commit. So if there is a change in the vendor we need to dad the file and commit it.

If this does not solve the problem I can take a look at it later

[vicmarbev]

What do you think of IsCobraManagedCommand?

💯 Makes sense to me.

could you help me with the IsCobraInternalCommand not declared by package error? I see no difference with the carvel-kapp version

@vicmarbev I don't see any diff in the vendor directory, you might wanna run go mod vendor or the ./hack/build.sh script.

That did it. Thanks!

[vicmarbev]

Anything else needed for this PR?

[joaopapereira]

@vicmarbev I am waiting on the PR https://github.com/cppforlife/cobrautil/pull/6 to get merged and when that is done I would ask you to bump the cobrautil library and I think we should be ready to go

[joaopapereira]

@vicmarbev whenever you can please bump cobrautil to the latest version so that we can use the new function. After that I think we will be ready to merge this PR