switch logging to slog

[jlambert121]

Description

Fixes #82.

Currently all logging is done via utils.Logger which just outputs logging to stdout. It would be nice to have more control over logging by silencing logging when not needed or injecting a separate logger (compatible with *slog.Logger).

There is a lot of lines of change here, but it's just creating a slog.Handler that prefixes [boilerplate] to log lines to maintain the existing look and passing an instance of the logger throughout. go is not my primary language though, any feedback is greatly appreciated!

NOTE: This does bump the minimum go version to 1.21.

TODOs

Read the Gruntwork contribution guidelines.

• Update the docs.
• Run the relevant tests successfully, including pre-commit checks.
• Ensure any 3rd party code adheres with our license policy or delete this line if its not applicable.
• Include release notes. If this PR is backward incompatible, include a migration guide.

Release Notes (draft)

Updated logging to use slog library.

Migration Guide

None

[Resonance1584]

Hi @jlambert121 , thanks for your contribution! This seems like a good improvement to Boilerplate.

I don't think BoilerplateOptions is the correct place for our Logger instance, that struct is specific to parsing configuration. There's two ways forward here:

1. Least work:

Use slog.SetDefault after parsing the --silent option, then use the slog.Info etc methods at log callsites. This is pretty similar to the way the existing logger is utilized, and allows us to use the correct logger in util/shell.go without modifying any function signatures.

2. More work:

Pass the instance of *slog.Logger everywhere as a function argument. Typically I make the first two arguments of my functions context.Context and Logger. This allows different code paths to construct new logger instances with different key val attributes. There's not much value in doing that here as we're not using log attributes, so its more of a style preference to avoid the global.

I'd suggest going with the first (least work) option for now and we can revisit in the future if we want to start using log attributes :)

[Resonance1584]

We'll need to use the same instance of the logger here, and below, to correctly respect the silent option.

[Resonance1584]

Lets use os.Stderr here, it's more typical for logs to be sent to STDERR whereas machine readable program output should go to STDOUT

[Resonance1584]

This feels weird because we're not using any of the attributes or grouping from the slog package - but this is still a net improvement allowing us to use --silent so I'm happy for this to go in as long as there's a TODO comment explaining it. Something like:

// TODO: Handle attributes and groups
_, err := fmt.Fprintf(h.writer, "[boilerplate] %s\n", r.Message)
return err

[Resonance1584]

Unless I'm reading this wrong, this mutex doesn't appear to be used and should be removed.

[Resonance1584]

This doesn't feel right, I think we should either use a global slog.Default() or pass the logger instance as a function argument everywhere.

[jlambert121]

@Resonance1584 thanks for taking a look at this!

I don't think BoilerplateOptions is the correct place for our Logger instance

Agree, was trying to limit the signature changes until there was some feedback though. In order to have an optional passed in logger (we're utilizing boilerplate inside another application), would you be opposed to changing the signature on ProcessTemplate to take a logger as well?

[Resonance1584]

@Resonance1584 thanks for taking a look at this!

I don't think BoilerplateOptions is the correct place for our Logger instance

Agree, was trying to limit the signature changes until there was some feedback though. In order to have an optional passed in logger (we're utilizing boilerplate inside another application), would you be opposed to changing the signature on ProcessTemplate to take a logger as well?

@jlambert121 I actually hadn't considered the use of this project as a library (thanks for raising that use case!) - which makes signature changes on exported functions problematic.

What we could do here is add a new exported function for your use case, and modify the existing function so that they go through the same code path without breaking compatibility.

ProcessTemplate(options, rootOpts *options.BoilerplateOptions, thisDep variables.Dependency) error {
  var l *slog.Logger
  if options.Silent {
    l = slog.New(slog.NewTextHandler(io.Discard, nil))
  } else {
    l = slog.New(prefixer.New())
  }
  return ProcessTemplateWithLogger(l, options, rootOpts, thisDep)
}

ProcessTemplateWithLogger(l *slog.Logger, options, rootOpts *options.BoilerplateOptions, thisDep variables.Dependency) error {
  // .. previous body of ProcessTemplate goes here
}

Would that support your use case?

Unfortunately we also won't be able to modify DownloadTemplatesToTemporaryFolder or RunShellCommand* without breaking compatibility, which I think means we would need to continue using a global like util/logger.go. Using slog.SetDefault would be really intuitive for external applications.

[jlambert121]

@jlambert121 I actually hadn't considered the use of this project as a library (thanks for raising that use case!) - which makes signature changes on exported functions problematic.

What we could do here is add a new exported function for your use case, and modify the existing function so that they go through the same code path without breaking compatibility.

Would that support your use case?

Unfortunately we also won't be able to modify DownloadTemplatesToTemporaryFolder or RunShellCommand* without breaking compatibility, which I think means we would need to continue using a global like util/logger.go. Using slog.SetDefault would be really intuitive for external applications.

Yeah, this seems like probably the best option - I'll see about refactoring with this. Yeah, DownloadTemplatesToTemporaryFolder and RunShellCommand are a bit more problematic.