Add --tail flag to stream logs with skaffold run#914
Add --tail flag to stream logs with skaffold run#914dgageot merged 3 commits intoGoogleContainerTools:masterfrom
Conversation
Shoulf fix GoogleContainerTools#253. Adds option to stream logs from objects deployed by skaffold run.
Codecov Report
@@ Coverage Diff @@
## master #914 +/- ##
==========================================
+ Coverage 39.01% 39.04% +0.02%
==========================================
Files 60 61 +1
Lines 2609 2638 +29
==========================================
+ Hits 1018 1030 +12
- Misses 1478 1495 +17
Partials 113 113
Continue to review full report at Codecov.
|
dgageot
left a comment
dgageot
left a comment
There was a problem hiding this comment.
A few questions. Other than that, it looks great!
pkg/skaffold/runner/runner.go
Outdated
| return errors.Wrap(err, "starting logger") | ||
| } | ||
|
|
||
| for { |
There was a problem hiding this comment.
Could this be replaced by just <-ctx.Done()?
There was a problem hiding this comment.
Also, should we setup the signal handling to handle ctrl-c properly?
There was a problem hiding this comment.
Oh yah good point, I'll replace it with <-ctx.Done()
For handling ctrl-c, I wasn't sure what the benefit of setting it up would be since there isn't any cleanup that needs to happen, the log stream just needs to end.
There was a problem hiding this comment.
Yeah, same for me. I think it's cleaner but I have no idea if it's really better
There was a problem hiding this comment.
Maybe we can add the ctrl-c handler to every command in another PR
cmd/skaffold/app/cmd/run.go
Outdated
| }, | ||
| } | ||
| AddRunDevFlags(cmd) | ||
| AddRunFlags(cmd) |
There was a problem hiding this comment.
Do you think also adding the flag to skaffold deploy would make sense?
There was a problem hiding this comment.
Yah I think that makes sense, I can add it there as well
| return errors.Wrap(err, "deploy step") | ||
| } | ||
|
|
||
| if !r.opts.Tail { |
There was a problem hiding this comment.
@priyawadhwa do you think the duplication could be removed?
There was a problem hiding this comment.
I thought about moving it into a separate function, but I think four things would have to be passed in, so it would look like:
func StreamLogsFromDeployedObjects(ctx context.Context, out io.Writer, builds []build.Artifact, artifacts []*v1alpha2.Artifact)and I personally didn't think that was much cleaner, but I don't have a strong opinion so can definitely move it into a function if you prefer that!
There was a problem hiding this comment.
Let's merge this one any think about how to remove the duplication afterwards!
|
Looks like kokoro failed downloading gcloud rebuilding |
4 participants
Shoulf fix #253. Adds option to stream logs from objects deployed by
skaffold run.