Refactor to use new SkaffoldWriter type #5894
Conversation
|
great PR! Can't wait for API v2! |
Codecov Report
@@ Coverage Diff @@
## master #5894 +/- ##
==========================================
- Coverage 70.90% 70.84% -0.07%
==========================================
Files 447 452 +5
Lines 16909 17321 +412
==========================================
+ Hits 11990 12271 +281
- Misses 4029 4146 +117
- Partials 890 904 +14
Continue to review full report at Codecov.
|
pkg/skaffold/output/output.go
Outdated
| if n < len(p) { | ||
| return n, io.ErrShortWrite | ||
| } |
There was a problem hiding this comment.
So n < |p| isn't an error: it happens pretty often when writing to a network stream.
There was a problem hiding this comment.
Do you think I should be handling this case at all then? ErrShortWrite means that a write accepted fewer bytes than requested but failed to return an explicit error. according to docs, so I thought I should handle it. It seems that most standard lib writers handle this case in the same way (although with !=, not <). If you think I shouldn't handle the case, i'll remove it, but if otherwise I'll change the condition to match the standard lib
| @@ -91,7 +91,7 @@ func (b *Builder) build(ctx context.Context, out io.Writer, a *latestV1.Artifact | |||
|
|
|||
| builderImage, runImage, pullPolicy := resolveDependencyImages(artifact, b.artifacts, a.Dependencies, b.pushImages) | |||
|
|
|||
| if err := runPackBuildFunc(ctx, output.GetWriter(out), b.localDocker, pack.BuildOptions{ | |||
| if err := runPackBuildFunc(ctx, output.GetUnderlyingWriter(out), b.localDocker, pack.BuildOptions{ | |||
There was a problem hiding this comment.
So doesn't this mean the output from this command won't go through our event logging?
Maybe we do this if there is no event writer?
There was a problem hiding this comment.
I was actually curious as to why we did this, and was going to address this in a follow up, as I believe we should be able to use our writer here. I'll probably need to make some more changes to the output setup that's happening in these functions
There was a problem hiding this comment.
The other thing to look at is the https://github.com/creack/pty package
| @@ -48,7 +48,7 @@ func (b *Builder) Build(ctx context.Context, out io.Writer, a *latestV1.Artifact | |||
| var imageID string | |||
|
|
|||
| if b.useCLI || b.useBuildKit { | |||
| imageID, err = b.dockerCLIBuild(ctx, output.GetWriter(out), a.Workspace, dockerfile, a.ArtifactType.DockerArtifact, opts) | |||
| imageID, err = b.dockerCLIBuild(ctx, output.GetUnderlyingWriter(out), a.Workspace, dockerfile, a.ArtifactType.DockerArtifact, opts) | |||
Related: #5370
Description
Introduces a new type,
SkaffoldWriter. This replacescolorableWriteras the top levelio.Writertype used by skaffold. It holds one main writer, which will most commonly be acolorableWriter, and a secondary writer, which will be used to write output out as events through skaffold API V2. The case for using this overio.MultiWriteris that this will allow us to implement functions that perform manipulations on the underlying writers that will be necessary for future implementation of API V2.Follow-up Work (remove if N/A)
Implement event writer type and use in this struct.