Fix buildpacks builder output to go through Event API #6530
Conversation
Codecov Report
@@ Coverage Diff @@
## main #6530 +/- ##
==========================================
- Coverage 70.44% 70.43% -0.01%
==========================================
Files 515 515
Lines 23144 23142 -2
==========================================
- Hits 16303 16300 -3
- Misses 5783 5784 +1
Partials 1058 1058
Continue to review full report at Codecov.
|
| @@ -20,10 +20,7 @@ import ( | |||
| "io" | |||
|
|
|||
| "github.com/buildpacks/pack/logging" | |||
| "github.com/mattn/go-colorable" | |||
| "github.com/sirupsen/logrus" | |||
There was a problem hiding this comment.
It would be great to remove the logrus from here and instead add a helper func NewLogger in output.Log.
Removing dependency on logger here will make the custom lint analyzer ignore list simpler.
See here https://github.com/GoogleContainerTools/skaffold/pull/6357/files#diff-82c7b02016fd4c13ff671a6b3c038aacdb4ed7aa4ec92871709e1c6bbaebe8d0R32
There was a problem hiding this comment.
Could be done as a follow up later.
There was a problem hiding this comment.
good idea, I can handle this in a follow up 👍🏼
| @@ -97,7 +96,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.GetUnderlyingWriter(out), b.localDocker, pack.BuildOptions{ | |||
| if err := runPackBuildFunc(ctx, out, b.localDocker, pack.BuildOptions{ | |||
There was a problem hiding this comment.
Would it make sense to plumb through a logger so that we get real log entries, rather than just logrus-formatted output strings on out?
There was a problem hiding this comment.
can you clarify what you mean by "real log entries" here?
There was a problem hiding this comment.
Pack uses its own logger implementation. With this change, we're passing in a logrus logger that's configured to write to this provided writer instance, which means we're losing information: the pack logging is being turned into Skaffold output, whereas we could pass the pack logging through as logging entries.
There was a problem hiding this comment.
Ah okay, I see what you mean. I'm not completely sure what the best approach for us would be here, as this change seems to be necessary for us to properly get the pack output through the event API stream
There was a problem hiding this comment.
this would probably be a good enhancement not just here, but for our other builder/deployers as well that use their own logging implementations. probably out of scope for this change though - can you add a TODO in the runPackBuild() function?
| @@ -97,7 +96,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.GetUnderlyingWriter(out), b.localDocker, pack.BuildOptions{ | |||
| if err := runPackBuildFunc(ctx, out, b.localDocker, pack.BuildOptions{ | |||
There was a problem hiding this comment.
this would probably be a good enhancement not just here, but for our other builder/deployers as well that use their own logging implementations. probably out of scope for this change though - can you add a TODO in the runPackBuild() function?
|
@nkubala I believe buildpacks builder is the only builder/deployer that has this issue, so we should be good from here |
Fixes: #6541
Description
The output we were sending to buildpacks client was stripped from our
output.SkaffoldWritertype, when we really do want to use that type. This PR fixes that. We should now see output from buildpacks builder in the Event API.I've removed the check for terminal on the io.Writer as well because this already happens at the beginning of execution.