Plumb context.Context down into functions that use util/cmd functions

[MarlonGamez]

Related: #5368

Description
This PR plumbs context.Context parameter into functions in order to have a context.Context variable in util.RunCmd and related functions

[codecov]

Codecov Report

Merging #6468 (0c227d7) into main (311b521) will decrease coverage by 0.03%.
The diff coverage is 78.68%.

@@            Coverage Diff             @@
##             main    #6468      +/-   ##
==========================================
- Coverage   70.29%   70.26%   -0.04%     
==========================================
  Files         510      510              
  Lines       22982    22985       +3     
==========================================
- Hits        16156    16151       -5     
- Misses       5770     5776       +6     
- Partials     1056     1058       +2     

Impacted Files	Coverage Δ
cmd/skaffold/skaffold.go	0.00% <0.00%> (ø)
pkg/skaffold/build/gcb/cloud_build.go	0.00% <0.00%> (ø)
pkg/skaffold/debug/apply_transforms.go	35.89% <0.00%> (ø)
pkg/skaffold/deploy/docker/deploy.go	0.00% <0.00%> (ø)
pkg/skaffold/diagnose/diagnose.go	15.78% <0.00%> (ø)
pkg/skaffold/docker/image_util.go	0.00% <0.00%> (ø)
pkg/skaffold/docker/logger/log.go	23.07% <0.00%> (ø)
pkg/skaffold/gcp/auth.go	26.47% <0.00%> (ø)
pkg/skaffold/gcp/client.go	0.00% <0.00%> (ø)
pkg/skaffold/initializer/analyze/directory.go	60.00% <0.00%> (ø)
... and 82 more

---

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 311b521...0c227d7. Read the comment docs.

[tejal29]

There is another PR to move replace context.Background to context.TODO.
Do you want to sync with that and see why TODO is preferred?

From docs, looks like Background context is never cancelled.
https://pkg.go.dev/context#Background

[gsquared94]

I think that context.TODO is a hint to the dev that if ever a proper context becomes available in future by a change in the function parameters or something, then plumb that through. Whereas context.Background is a deliberate use even if a parent context is available. That's what I understood from @ahmetb's PR.

[ahmetb]

yeah that's the case. ideally a context.Background() is for a call stack that can be cancelled from a central place, e.g. Ctrl-C. Background threads with separate lifecycle (as the name also says) get a new Background context.

If you don't plumb yet but intend do plumb someday, you can use context.TODO, which is identical to Background() functionally, only different in semantics and indicates your intention.

[tejal29]

Code changes Look good.
Trying this out.

[tejal29]

After applying a diff and some validation I can now see debug logs in relevant nodes.
e.g. skaffold deploy logs are not seen in deploy node for skaffold run config with debug level logging.

Diff i applied:

diff --git a/pkg/skaffold/event/v2/logger.go b/pkg/skaffold/event/v2/logger.go
index d38e68a8c..bf19f8e9c 100644
--- a/pkg/skaffold/event/v2/logger.go
+++ b/pkg/skaffold/event/v2/logger.go
@@ -86,9 +86,16 @@ func (h logHook) Levels() []logrus.Level {
 
 // Fire constructs a SkaffoldLogEvent and sends it to the event channel
 func (h logHook) Fire(entry *logrus.Entry) error {
+       task := constants.DevLoop
+       if s, ok := entry.Data["task"]; ok {
+               fmt.Println("found for entry", entry.Message)
+               task = s.(constants.Phase)
+       } else {
+               fmt.Println("not found for ", entry.Message, entry.Data)
+       }
        handler.handleSkaffoldLogEvent(&proto.SkaffoldLogEvent{
-               TaskId:    fmt.Sprintf("%s-%d", h.task, handler.iteration),
-               SubtaskId: h.subtask,
+               TaskId:    fmt.Sprintf("%s-%d", task, handler.iteration),
+               SubtaskId: entry.Data["subtask"].(string),
                Level:     levelFromEntry(entry),
                Message:   entry.Message,
        })

Please apply the diff to make sure the context values are plumbed through

[tejal29]

Testing for BUILD scenario.

Seeing mvn build command in the build node for frontend and backend.

[tejal29]

Portfowarding logs are in the right section

[tejal29]

Follow up cleaning tasks

1. output.WithContext should also return the traceContext and endTrace or instrumentation.StartTrace should preserve context values.
   right now, instrumentation.StartTrace(ctx, "Cleanup") clears all context values if tracing if enabled.

[tejal29]

LGTM for Monday in advance.

[gsquared94]

can this be merged @MarlonGamez ?