Attaching os standard error and out stream to the copy command

[prary]

Copy command doesn't output the logs to the user, hence attaching logs to stderr and stdout.

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

[codecov-io]

Codecov Report

Merging #1960 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1960      +/-   ##
==========================================
- Coverage   52.58%   52.56%   -0.02%     
==========================================
  Files         180      180              
  Lines        7898     7900       +2     
==========================================
  Hits         4153     4153              
- Misses       3356     3358       +2     
  Partials      389      389

Impacted Files	Coverage Δ
pkg/skaffold/sync/kubectl/kubectl.go	9.09% <0%> (-0.59%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 30da253...29f3abb. Read the comment docs.

[corneliusweig]

@prary I don't think it is generally acceptable to simply forward the output to stdout/stderr. Error logging should be done with the logger. In addition, the deleteFileFn has the same problem. What do you think about this approach:

diff --git a/pkg/skaffold/sync/kubectl/kubectl.go b/pkg/skaffold/sync/kubectl/kubectl.go
index 836e6252..6ba56f2f 100644
--- a/pkg/skaffold/sync/kubectl/kubectl.go
+++ b/pkg/skaffold/sync/kubectl/kubectl.go
@@ -19,7 +19,6 @@ package kubectl
 import (
 	"context"
 	"io"
-	"os"
 	"os/exec"
 
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/sync"
@@ -77,9 +76,6 @@ func copyFileFn(ctx context.Context, pod v1.Pod, container v1.Container, files m
 	copy := exec.CommandContext(ctx, "kubectl", "exec", pod.Name, "--namespace", pod.Namespace, "-c", container.Name, "-i",
 		"--", "tar", "xmf", "-", "-C", "/", "--no-same-owner")
 	copy.Stdin = reader
-	copy.Stdout = os.Stdout
-	copy.Stderr = os.Stderr
-
 	go func() {
 		defer writer.Close()
 
diff --git a/pkg/skaffold/sync/sync.go b/pkg/skaffold/sync/sync.go
index 1994fe77..93b0bf14 100644
--- a/pkg/skaffold/sync/sync.go
+++ b/pkg/skaffold/sync/sync.go
@@ -17,6 +17,7 @@ limitations under the License.
 package sync
 
 import (
+	"bytes"
 	"context"
 	"fmt"
 	"os/exec"
@@ -247,7 +248,11 @@ func Perform(ctx context.Context, image string, files map[string]string, cmdFn f
 
 				cmds := cmdFn(ctx, p, c, files)
 				for _, cmd := range cmds {
+					buf := &bytes.Buffer{}
+					cmd.Stdout = buf
+					cmd.Stderr = buf
 					if err := util.RunCmd(cmd); err != nil {
+						logrus.Warnf("Sync failed: %s", buf.String())
 						return err
 					}
 					numSynced++

[prary]

+					buf := &bytes.Buffer{}
+					cmd.Stdout = buf
+					cmd.Stderr = buf
 					if err := util.RunCmd(cmd); err != nil {
+						logrus.Warnf("Sync failed: %s", buf.String())

@corneliusweig this one is even better and cover delete operation also, will incorporate the changes.

Thanks for the enhancement.

[googlebot]

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

[corneliusweig]

@prary This currently does not build.

However, I think that the suggestion from @dgageot to use RunCmdOut would actually not work here, because stderr is swallowed by RunCmdOut. And at least on my system, tar and rm log errors to stderr. So in essence, the error output from the sync commands would not be shown.

I didn't realize that both stdout and stderr will be put into the returned error. So all good with this approach!

[corneliusweig]

stdout should be completely uninteresting here. It is just the output of rm or tar xf, which do not print to stdout. And even if they did, it would just clutter the skaffold log IMHO

[briandealwis]

This is great. If there's an error (for example, the non-debug distroless images don't include tar), then the user sees an warning:

WARN[0021] Skipping deploy due to sync error: copying files: Running [kubectl exec web-64df5fc8c7-p5p5j --namespace default -c web -i -- tar xmf - -C / --no-same-owner]: stdout OCI runtime exec failed: exec failed: container_linux.go:344: starting container process caused "exec: \"tar\": executable file not found in $PATH": unknown
, stderr: command terminated with exit code 126
, err: exit status 126: exit status 126 

[prary]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

Googlers: Go here for more info.

I signed it!

addressed