fix(logging): eliminate global slog usage per original PR intent

- Add NewLogIgnorePluginLoadErrorFilterFunc(logger) factory so plugin load
  warnings are routed through the DI logger instead of global slog
- Add FindPluginsWithErrorFilter to thread logger through CLI plugin loading
- Update loadCLIPlugins, plugin_list, plugin_uninstall, plugin_update to use
  DI logger for plugin load warnings
- Add WithLogger option to getter for HTTP fetch debug logging
- Remove slog.SetDefault() from newRootCmdWithConfig; global slog is no longer
  mutated by the CLI
- Replace slog.Default() fallbacks with slog.New(slog.DiscardHandler) in
  public API functions whose signatures cannot change until Helm v5
  (ProcessDependencies, ValidateAgainstSchema, ValidateAgainstSingleSchema,
  IndexFile.log)

Signed-off-by: JaeguKim <rlaworn1993@gmail.com>
pull/31833/head
JaeguKim 5 months ago
parent c65584458c
commit 10e993a5ec

@ -30,7 +30,7 @@ import (
// ProcessDependencies checks through this chart's dependencies, processing accordingly. // ProcessDependencies checks through this chart's dependencies, processing accordingly.
func ProcessDependencies(c *chart.Chart, v common.Values) error { func ProcessDependencies(c *chart.Chart, v common.Values) error {
logger := slog.Default() logger := slog.New(slog.DiscardHandler)
if err := processDependencyEnabled(c, v, "", logger); err != nil { if err := processDependencyEnabled(c, v, "", logger); err != nil {
return err return err
} }

@ -159,9 +159,19 @@ func LoadDir(dirname string) (Plugin, error) {
return pm.CreatePlugin(dirname, m) return pm.CreatePlugin(dirname, m)
} }
// NewLogIgnorePluginLoadErrorFilterFunc returns an ErrorFilterFunc that logs
// plugin load errors using the provided logger and ignores them.
func NewLogIgnorePluginLoadErrorFilterFunc(logger *slog.Logger) ErrorFilterFunc {
return func(pluginYAML string, err error) error {
logger.Warn("failed to load plugin (ignoring)", slog.String("plugin_yaml", pluginYAML), slog.Any("error", err))
return nil
}
}
// LogIgnorePluginLoadErrorFilterFunc logs plugin load errors and ignores them.
// Deprecated: Use NewLogIgnorePluginLoadErrorFilterFunc to inject a logger.
func LogIgnorePluginLoadErrorFilterFunc(pluginYAML string, err error) error { func LogIgnorePluginLoadErrorFilterFunc(pluginYAML string, err error) error {
slog.Warn("failed to load plugin (ignoring)", slog.String("plugin_yaml", pluginYAML), slog.Any("error", err)) return NewLogIgnorePluginLoadErrorFilterFunc(slog.New(slog.DiscardHandler))(pluginYAML, err)
return nil
} }
// errorFilterFunc is a function that can filter errors during plugin loading // errorFilterFunc is a function that can filter errors during plugin loading
@ -205,13 +215,20 @@ type findFunc func(pluginsDir string) ([]Plugin, error)
// filterFunc is a function that filters plugins // filterFunc is a function that filters plugins
type filterFunc func(Plugin) bool type filterFunc func(Plugin) bool
// FindPlugins returns a list of plugins that match the descriptor // FindPluginsWithErrorFilter returns a list of plugins that match the descriptor,
// Errors loading a plugin are ignored with a warning // using the provided error filter to handle individual plugin load errors.
func FindPlugins(pluginsDirs []string, descriptor Descriptor) ([]Plugin, error) { func FindPluginsWithErrorFilter(pluginsDirs []string, descriptor Descriptor, errFilter ErrorFilterFunc) ([]Plugin, error) {
loadAllIgnoreErrors := func(pluginsDir string) ([]Plugin, error) { loadAll := func(pluginsDir string) ([]Plugin, error) {
return LoadAllDir(pluginsDir, LogIgnorePluginLoadErrorFilterFunc) return LoadAllDir(pluginsDir, errFilter)
} }
return findPlugins(pluginsDirs, loadAllIgnoreErrors, makeDescriptorFilter(descriptor)) return findPlugins(pluginsDirs, loadAll, makeDescriptorFilter(descriptor))
}
// FindPlugins returns a list of plugins that match the descriptor.
// Errors loading a plugin are silently ignored.
// Deprecated: Use FindPluginsWithErrorFilter to handle load errors explicitly.
func FindPlugins(pluginsDirs []string, descriptor Descriptor) ([]Plugin, error) {
return FindPluginsWithErrorFilter(pluginsDirs, descriptor, LogIgnorePluginLoadErrorFilterFunc)
} }
// findPlugins is the internal implementation that uses the find and filter functions // findPlugins is the internal implementation that uses the find and filter functions

@ -74,7 +74,7 @@ func newHTTPURLLoader() *HTTPURLLoader {
// ValidateAgainstSchema checks that values does not violate the structure laid out in schema // ValidateAgainstSchema checks that values does not violate the structure laid out in schema
func ValidateAgainstSchema(ch chart.Charter, values map[string]any) error { func ValidateAgainstSchema(ch chart.Charter, values map[string]any) error {
logger := slog.Default() logger := slog.New(slog.DiscardHandler)
chrt, err := chart.NewAccessor(ch) chrt, err := chart.NewAccessor(ch)
if err != nil { if err != nil {
return err return err
@ -123,7 +123,7 @@ func ValidateAgainstSchema(ch chart.Charter, values map[string]any) error {
// ValidateAgainstSingleSchema checks that values does not violate the structure laid out in this schema // ValidateAgainstSingleSchema checks that values does not violate the structure laid out in this schema
func ValidateAgainstSingleSchema(values common.Values, schemaJSON []byte) (reterr error) { func ValidateAgainstSingleSchema(values common.Values, schemaJSON []byte) (reterr error) {
logger := slog.Default() logger := slog.New(slog.DiscardHandler)
defer func() { defer func() {
if r := recover(); r != nil { if r := recover(); r != nil {
reterr = fmt.Errorf("unable to validate schema: %s", r) reterr = fmt.Errorf("unable to validate schema: %s", r)

@ -30,7 +30,7 @@ import (
// ProcessDependencies checks through this chart's dependencies, processing accordingly. // ProcessDependencies checks through this chart's dependencies, processing accordingly.
func ProcessDependencies(c *chart.Chart, v common.Values) error { func ProcessDependencies(c *chart.Chart, v common.Values) error {
logger := slog.Default() logger := slog.New(slog.DiscardHandler)
if err := processDependencyEnabled(c, v, "", logger); err != nil { if err := processDependencyEnabled(c, v, "", logger); err != nil {
return err return err
} }

@ -62,7 +62,7 @@ func loadCLIPlugins(baseCmd *cobra.Command, out io.Writer, logger *slog.Logger)
descriptor := plugin.Descriptor{ descriptor := plugin.Descriptor{
Type: "cli/v1", Type: "cli/v1",
} }
found, err := plugin.FindPlugins(dirs, descriptor) found, err := plugin.FindPluginsWithErrorFilter(dirs, descriptor, plugin.NewLogIgnorePluginLoadErrorFilterFunc(logger))
if err != nil { if err != nil {
logger.Error("failed to load plugins", slog.String("error", err.Error())) logger.Error("failed to load plugins", slog.String("error", err.Error()))
return return

@ -42,7 +42,7 @@ func newPluginListCmd(out io.Writer, logger *slog.Logger) *cobra.Command {
descriptor := plugin.Descriptor{ descriptor := plugin.Descriptor{
Type: pluginType, Type: pluginType,
} }
plugins, err := plugin.FindPlugins(dirs, descriptor) plugins, err := plugin.FindPluginsWithErrorFilter(dirs, descriptor, plugin.NewLogIgnorePluginLoadErrorFilterFunc(logger))
if err != nil { if err != nil {
return err return err
} }

@ -63,7 +63,7 @@ func (o *pluginUninstallOptions) complete(args []string) error {
func (o *pluginUninstallOptions) run(out io.Writer) error { func (o *pluginUninstallOptions) run(out io.Writer) error {
o.logger.Debug("loading installer plugins", "dir", settings.PluginsDirectory) o.logger.Debug("loading installer plugins", "dir", settings.PluginsDirectory)
plugins, err := plugin.LoadAllDir(settings.PluginsDirectory, plugin.LogIgnorePluginLoadErrorFilterFunc) plugins, err := plugin.LoadAllDir(settings.PluginsDirectory, plugin.NewLogIgnorePluginLoadErrorFilterFunc(o.logger))
if err != nil { if err != nil {
return err return err
} }

@ -63,7 +63,7 @@ func (o *pluginUpdateOptions) complete(args []string) error {
func (o *pluginUpdateOptions) run(out io.Writer) error { func (o *pluginUpdateOptions) run(out io.Writer) error {
o.logger.Debug("loading installed plugins", "path", settings.PluginsDirectory) o.logger.Debug("loading installed plugins", "path", settings.PluginsDirectory)
plugins, err := plugin.LoadAllDir(settings.PluginsDirectory, plugin.LogIgnorePluginLoadErrorFilterFunc) plugins, err := plugin.LoadAllDir(settings.PluginsDirectory, plugin.NewLogIgnorePluginLoadErrorFilterFunc(o.logger))
if err != nil { if err != nil {
return err return err
} }

@ -177,7 +177,6 @@ func newRootCmdWithConfig(actionConfig *action.Configuration, out io.Writer, arg
flags.Parse(args) flags.Parse(args)
logger := logSetup(settings.Debug) logger := logSetup(settings.Debug)
slog.SetDefault(logger)
actionConfig.SetLogger(logger.Handler()) actionConfig.SetLogger(logger.Handler())
// Validate color mode setting // Validate color mode setting

@ -149,9 +149,9 @@ func TestRootCmdLogger(t *testing.T) {
t.Error("expected actionConfig logger to be set, got discard handler") t.Error("expected actionConfig logger to be set, got discard handler")
} }
// slog.SetDefault is called so the global default should use the same handler // slog.SetDefault is not called, so the global default should differ from the actionConfig logger
if l.Handler() != slog.Default().Handler() { if l.Handler() == slog.Default().Handler() {
t.Error("expected actionConfig logger to match the global slog default logger") t.Error("expected actionConfig logger to differ from the global slog default logger")
} }
} }

@ -19,6 +19,7 @@ package getter
import ( import (
"bytes" "bytes"
"fmt" "fmt"
"log/slog"
"net/http" "net/http"
"slices" "slices"
"time" "time"
@ -49,6 +50,7 @@ type getterOptions struct {
timeout time.Duration timeout time.Duration
transport *http.Transport transport *http.Transport
artifactType string artifactType string
logger *slog.Logger
} }
// Option allows specifying various settings configurable by the user for overriding the defaults // Option allows specifying various settings configurable by the user for overriding the defaults
@ -152,6 +154,13 @@ func WithArtifactType(artifactType string) Option {
} }
} }
// WithLogger sets the logger for the getter.
func WithLogger(logger *slog.Logger) Option {
return func(opts *getterOptions) {
opts.logger = logger
}
}
// Getter is an interface to support GET to the specified URL. // Getter is an interface to support GET to the specified URL.
type Getter interface { type Getter interface {
// Get file content by url string // Get file content by url string

@ -88,13 +88,17 @@ func (g *HTTPGetter) get(href string, opts getterOptions) (*bytes.Buffer, error)
return nil, err return nil, err
} }
slog.Debug("fetching", "url", href) logger := opts.logger
if logger == nil {
logger = slog.New(slog.DiscardHandler)
}
logger.Debug("fetching", "url", href)
resp, err := client.Do(req) resp, err := client.Do(req)
if err != nil { if err != nil {
return nil, err return nil, err
} }
defer resp.Body.Close() defer resp.Body.Close()
slog.Debug("fetch complete", "url", href, "status", resp.Status, "content-length", resp.ContentLength) logger.Debug("fetch complete", "url", href, "status", resp.Status, "content-length", resp.ContentLength)
if resp.StatusCode != http.StatusOK { if resp.StatusCode != http.StatusOK {
return nil, fmt.Errorf("failed to fetch %s : %s", href, resp.Status) return nil, fmt.Errorf("failed to fetch %s : %s", href, resp.Status)
} }

@ -99,7 +99,7 @@ func (i IndexFile) log() *slog.Logger {
if i.Logger != nil { if i.Logger != nil {
return i.Logger return i.Logger
} }
return slog.Default() return slog.New(slog.DiscardHandler)
} }
// NewIndexFile initializes an index. // NewIndexFile initializes an index.

Loading…
Cancel
Save