Expose LeveledLogger's Writers

[fg-j]

We discussed the topic of log streaming at a working group meeting and it resulted in a change to the Paketo Buildpacks Style Guide, which now states:

If a buildpack invokes a tool that produces logs (e.g. npm install), it is preferred that the buildpack stream the logs (rather than buffering/swallowing them) to provide maximum visibility to users. The buildpack may adjust the verbosity of a tool's logs or apply formatting to them to improve the overall build logs' readability.

This captures the sentiment of buildpack maintainers that streaming logs is preferable as log as the resulting bulid output is readable overall.

So I've tweaked my approach in the dotnet-publish buildpack to simply stream the output of dotnet publish in all cases.

I chose to expose scribe.Emitter.ActionWriter so that the log output from dotnet publish appears in a format consistent with other buildpack output, without breaking the current log formatting abstraction. To achieve consistent formatting using the scribe.Writer directly, I'd need to dig into the implementation of scribe.Emitter.Action() to see how the logs are formatted. I think it's better to abstract log formatting away from buildpack authors as much as possible.

[ryanmoran]

I'm completely onboard with the intention of this change. I'm also wondering if we might be able to find an API that isn't so repetitive. I threw this together, but I'm not convinced that it isn't too clever by half.

It would let us just keep the existing public API we have, but allow the functions to be used as io.Writer types directly. For example, you can pass the reference to logger.Action as the io.Writer to stream output to:

logger := scribe.NewLogger(os.Stdout)
executable := pexec.NewExecutable("bundle")
err := executable.Execute(pexec.Execution{
  Args: []string{"install"},
  Stdout: logger.Action,
})
if err != nil {
  panic(err)
}

// This would print the output from `bundle install` indented at the "Action" level.

I like the simplicity of the API in @fg-j's implementation though.

@paketo-buildpacks/tooling-maintainers, your thoughts?

[fg-j]

@ryanmoran I'm curious how your version changes the current syntax we have for "regular" logging, like

logger.Action("The thing I want to say")

Currently on main, that call writes the input string to the stream and returns nothing. From what I can tell, in your edit, that call would skip writing the input string and return the writer.
To log the same string, it looks like you'd need to:

logger.Action.Write([]byte("The thing I want to say"))

Is this an intentional change? Or am I missing something?

[sophiewigmore]

I find @fg-j's implementation a bit easier to follow and don't mind the repetitiveness in favour of readability, I think I prefer that. It makes things a bit easier for buildpack authors trying to understand packit

[ryanmoran]

@ryanmoran I'm curious how your version changes the current syntax we have for "regular" logging, like

logger.Action("The thing I want to say")

Currently on main, that call writes the input string to the stream and returns nothing. From what I can tell, in your edit, that call would skip writing the input string and return the writer. To log the same string, it looks like you'd need to:

logger.Action.Write([]byte("The thing I want to say"))

Is this an intentional change? Or am I missing something?

That's not what happens. As you can see in these tests the existing behavior is maintained.

packit/scribe/logger_test.go

Lines 29 to 32 in 7c82261

	it("prints the output with no indentation", func() {
	logger.Title("some-%s", "title")
	Expect(buffer.String()).To(Equal("some-title\n"))
	})

[ForestEckhardt]

@ryanmoran I think that I am following the logic you have purposed but if you could walk through and the introduction of the _fw struct and the skip logic check because that is where the tires are falling off for me. I am not entirely sure what these purpose of these new constructs are.

[ryanmoran]

The fact that this is requiring so much explanation and creating clear confusion makes me think that my assumptions were correct. This is too clever. It creates a pretty API at the expense of maintainability. I propose that we drop it from consideration.