From 27f687ce166a35ae198debebbeb117b421346b6a Mon Sep 17 00:00:00 2001 From: Christopher Phillips Date: Mon, 5 Feb 2024 11:10:01 -0500 Subject: [PATCH 1/4] fix: remove panic case from conflicting configs Signed-off-by: Christopher Phillips --- application.go | 15 +++++++++------ application_test.go | 8 ++++---- cliotestutils/application.go | 4 ++-- help_template.go | 23 ++++++++++++++++------- help_template_test.go | 11 ++++++++++- setup_config.go | 6 ++++-- 6 files changed, 45 insertions(+), 22 deletions(-) diff --git a/application.go b/application.go index 2adb046..2d9c30a 100644 --- a/application.go +++ b/application.go @@ -23,13 +23,13 @@ type Initializer func(*State) error type PostRun func(*State, error) -type postConstruct func(*application) +type postConstruct func(*application) error type Application interface { ID() Identification AddFlags(flags *pflag.FlagSet, cfgs ...any) SetupCommand(cmd *cobra.Command, cfgs ...any) *cobra.Command - SetupRootCommand(cmd *cobra.Command, cfgs ...any) *cobra.Command + SetupRootCommand(cmd *cobra.Command, cfgs ...any) (*cobra.Command, error) Run() } @@ -276,12 +276,12 @@ func (a *application) Run() { } } -func (a *application) SetupRootCommand(cmd *cobra.Command, cfgs ...any) *cobra.Command { +func (a *application) SetupRootCommand(cmd *cobra.Command, cfgs ...any) (*cobra.Command, error) { a.root = cmd return a.setupRootCommand(cmd, cfgs...) } -func (a *application) setupRootCommand(cmd *cobra.Command, cfgs ...any) *cobra.Command { +func (a *application) setupRootCommand(cmd *cobra.Command, cfgs ...any) (*cobra.Command, error) { if !strings.HasPrefix(cmd.Use, a.setupConfig.ID.Name) { cmd.Use = a.setupConfig.ID.Name } @@ -295,10 +295,13 @@ func (a *application) setupRootCommand(cmd *cobra.Command, cfgs ...any) *cobra.C a.state.Config.Dev = cp(a.setupConfig.DefaultDevelopmentConfig) for _, pc := range a.setupConfig.postConstructs { - pc(a) + err := pc(a) + if err != nil { + return nil, err + } } - return a.setupCommand(cmd, cmd.Flags(), &cmd.PreRunE, cfgs...) + return a.setupCommand(cmd, cmd.Flags(), &cmd.PreRunE, cfgs...), nil } func cp[T any](value *T) *T { diff --git a/application_test.go b/application_test.go index 1a53d04..79717b5 100644 --- a/application_test.go +++ b/application_test.go @@ -127,7 +127,7 @@ func Test_Application_Setup_WiresFangs(t *testing.T) { app := New(*cfg) - cmd := app.SetupRootCommand(&cobra.Command{ + cmd, _ := app.SetupRootCommand(&cobra.Command{ DisableFlagParsing: true, Args: cobra.ArbitraryArgs, Run: func(cmd *cobra.Command, args []string) { @@ -163,7 +163,7 @@ func Test_Application_Setup_PassLoggerConstructor(t *testing.T) { app := New(*cfg) - cmd := app.SetupRootCommand(&cobra.Command{ + cmd, _ := app.SetupRootCommand(&cobra.Command{ DisableFlagParsing: true, Args: cobra.ArbitraryArgs, Run: func(cmd *cobra.Command, args []string) {}, @@ -192,7 +192,7 @@ func Test_Application_Setup_ConfigureLogger(t *testing.T) { app := New(*cfg) - cmd := app.SetupRootCommand(&cobra.Command{ + cmd, _ := app.SetupRootCommand(&cobra.Command{ DisableFlagParsing: true, Args: cobra.ArbitraryArgs, Run: func(cmd *cobra.Command, args []string) {}, @@ -280,7 +280,7 @@ func Test_SetupCommand(t *testing.T) { app := New(*cfg) - root := app.SetupRootCommand(&cobra.Command{}) + root, _ := app.SetupRootCommand(&cobra.Command{}) app.AddFlags(root.PersistentFlags(), p) diff --git a/cliotestutils/application.go b/cliotestutils/application.go index d69394e..ca4c1ee 100644 --- a/cliotestutils/application.go +++ b/cliotestutils/application.go @@ -58,12 +58,12 @@ func (a *testApplication) SetupCommand(cmd *cobra.Command, cfgs ...any) *cobra.C return a.Application.SetupCommand(cmd, cfgs...) } -func (a *testApplication) SetupRootCommand(cmd *cobra.Command, cfgs ...any) *cobra.Command { +func (a *testApplication) SetupRootCommand(cmd *cobra.Command, cfgs ...any) (*cobra.Command, error) { cmd.RunE = func(cmd *cobra.Command, args []string) error { a.assertion(cmd, args, cfgs...) return nil } - return a.Application.SetupRootCommand(cmd, cfgs...) + return a.Application.SetupRootCommand(cmd, cfgs...), nil } /* diff --git a/help_template.go b/help_template.go index c1cf3d4..7e0a4e5 100644 --- a/help_template.go +++ b/help_template.go @@ -2,7 +2,7 @@ package clio import ( "fmt" - "reflect" + "log" "github.com/spf13/cobra" @@ -11,7 +11,7 @@ import ( var _ postConstruct = updateHelpUsageTemplate -func updateHelpUsageTemplate(a *application) { +func updateHelpUsageTemplate(a *application) error { cmd := a.root var helpUsageTemplate = fmt.Sprintf(`{{if (or .Long .Short)}}{{.Long}}{{if not .Long}}{{.Short}}{{end}} @@ -42,27 +42,35 @@ Use "{{if .CommandPath}}{{.CommandPath}} {{end}}[command] --help" for more infor cmd.SetUsageTemplate(helpUsageTemplate) cmd.SetHelpTemplate(helpUsageTemplate) + return nil } var _ postConstruct = showConfigInRootHelp -func showConfigInRootHelp(a *application) { +func showConfigInRootHelp(a *application) error { cmd := a.root helpFn := cmd.HelpFunc() + // check if the config can load before setting the help function + cfgs := append([]any{&a.state.Config, a}, a.state.Config.FromCommands...) + for _, cfg := range cfgs { + if err := fangs.Load(a.setupConfig.FangsConfig, cmd, cfg); err != nil { + return fmt.Errorf("error loading config object; do you have a config that conflicts with a newer version? %w", err) + } + } cmd.SetHelpFunc(func(cmd *cobra.Command, args []string) { // root.Example is set _after all added commands_ because it collects all the // options structs in order to output an accurate "config file" summary // note: since all commands tend to share help functions it's important to only patch the example // when there is no parent command (i.e. the root command). if cmd == a.root { - cfgs := append([]any{&a.state.Config, a}, a.state.Config.FromCommands...) - for _, cfg := range cfgs { + helpCfgs := append([]any{&a.state.Config, a}, a.state.Config.FromCommands...) + for _, cfg := range helpCfgs { // load each config individually, as there may be conflicting names / types that will cause // viper to fail to read them all and panic if err := fangs.Load(a.setupConfig.FangsConfig, cmd, cfg); err != nil { - t := reflect.TypeOf(cfg) - panic(fmt.Sprintf("error loading config object: `%s:%s`: %s", t.PkgPath(), t.Name(), err.Error())) + // log default prints to stderr + log.Printf("error loading config object; do you have a config that conflicts with a newer version? %v", err) } } summary := a.summarizeConfig(cmd) @@ -73,4 +81,5 @@ func showConfigInRootHelp(a *application) { } helpFn(cmd, args) }) + return nil } diff --git a/help_template_test.go b/help_template_test.go index 54aad7e..5bf05f7 100644 --- a/help_template_test.go +++ b/help_template_test.go @@ -12,6 +12,15 @@ import ( "github.com/anchore/go-logger/adapter/redact" ) +func Test_showConfigInRootHelp(t *testing.T) { + cfg := NewSetupConfig(Identification{ + Name: "app", + Version: "1.2.3", + }). + WithConfigInRootHelp(). + WithGlobalConfigFlag() +} + func Test_redactingHelpText(t *testing.T) { cfg := NewSetupConfig(Identification{ Name: "app", @@ -27,7 +36,7 @@ func Test_redactingHelpText(t *testing.T) { app := New(*cfg) - root := app.SetupRootCommand(&cobra.Command{}, r) + root, _ := app.SetupRootCommand(&cobra.Command{}, r) r.store = app.(*application).state.RedactStore diff --git a/setup_config.go b/setup_config.go index a39bf93..e3241d4 100644 --- a/setup_config.go +++ b/setup_config.go @@ -110,15 +110,17 @@ func (c *SetupConfig) withPostConstructs(postConstructs ...postConstruct) *Setup // WithGlobalConfigFlag adds the global `-c` / `--config` flags to the root command func (c *SetupConfig) WithGlobalConfigFlag() *SetupConfig { - return c.withPostConstructs(func(a *application) { + return c.withPostConstructs(func(a *application) error { a.AddFlags(a.root.PersistentFlags(), &a.setupConfig.FangsConfig) + return nil }) } // WithGlobalLoggingFlags adds the global logging flags to the root command. func (c *SetupConfig) WithGlobalLoggingFlags() *SetupConfig { - return c.withPostConstructs(func(a *application) { + return c.withPostConstructs(func(a *application) error { a.AddFlags(a.root.PersistentFlags(), &a.state.Config) + return nil }) } From 6541b6fdfa809661080a0896a2126fbb73dae4d5 Mon Sep 17 00:00:00 2001 From: Christopher Phillips Date: Mon, 5 Feb 2024 11:33:17 -0500 Subject: [PATCH 2/4] chore: fix refactor Signed-off-by: Christopher Phillips --- cliotestutils/application.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cliotestutils/application.go b/cliotestutils/application.go index ca4c1ee..2293204 100644 --- a/cliotestutils/application.go +++ b/cliotestutils/application.go @@ -63,7 +63,7 @@ func (a *testApplication) SetupRootCommand(cmd *cobra.Command, cfgs ...any) (*co a.assertion(cmd, args, cfgs...) return nil } - return a.Application.SetupRootCommand(cmd, cfgs...), nil + return a.Application.SetupRootCommand(cmd, cfgs...) } /* From 40507c8aedeb15b1c43bd48de3462c0ad9ca3496 Mon Sep 17 00:00:00 2001 From: Christopher Phillips Date: Mon, 5 Feb 2024 11:39:41 -0500 Subject: [PATCH 3/4] fix: add early return on load failure Signed-off-by: Christopher Phillips --- help_template.go | 1 + 1 file changed, 1 insertion(+) diff --git a/help_template.go b/help_template.go index 7e0a4e5..f95d2a5 100644 --- a/help_template.go +++ b/help_template.go @@ -71,6 +71,7 @@ func showConfigInRootHelp(a *application) error { if err := fangs.Load(a.setupConfig.FangsConfig, cmd, cfg); err != nil { // log default prints to stderr log.Printf("error loading config object; do you have a config that conflicts with a newer version? %v", err) + return } } summary := a.summarizeConfig(cmd) From cc641a75e0a64df7fcbfcf15fede72563845cd7b Mon Sep 17 00:00:00 2001 From: Christopher Phillips Date: Mon, 5 Feb 2024 12:10:33 -0500 Subject: [PATCH 4/4] test: add regression for conflicting configs Signed-off-by: Christopher Phillips --- help_template_test.go | 29 ++++++++++++++++++++++------- testdata/conflicting_config.yaml | 3 +++ 2 files changed, 25 insertions(+), 7 deletions(-) create mode 100644 testdata/conflicting_config.yaml diff --git a/help_template_test.go b/help_template_test.go index 5bf05f7..0f7de44 100644 --- a/help_template_test.go +++ b/help_template_test.go @@ -9,16 +9,31 @@ import ( "github.com/spf13/cobra" "github.com/stretchr/testify/require" + "github.com/anchore/fangs" "github.com/anchore/go-logger/adapter/redact" ) -func Test_showConfigInRootHelp(t *testing.T) { - cfg := NewSetupConfig(Identification{ - Name: "app", - Version: "1.2.3", - }). - WithConfigInRootHelp(). - WithGlobalConfigFlag() +// regression test for https://github.com/anchore/clio/issues/40 +func Test_conflictingConfigErrors(t *testing.T) { + fangsConfig := fangs.NewConfig("test") + fangsConfig.File = "testdata/conflicting_config.yaml" + application := &application{ + root: &cobra.Command{}, + state: State{ + Config: Config{ + FromCommands: []any{&struct { + UnderTest string `mapstructure:"test" yaml:"test"` + }{UnderTest: "test"}}, + }, + }, + setupConfig: SetupConfig{ + FangsConfig: fangsConfig, + }, + } + err := showConfigInRootHelp(application) + require.Error(t, err) + msg := err.Error() + require.Contains(t, msg, "error loading config object") } func Test_redactingHelpText(t *testing.T) { diff --git a/testdata/conflicting_config.yaml b/testdata/conflicting_config.yaml new file mode 100644 index 0000000..a0cbe9a --- /dev/null +++ b/testdata/conflicting_config.yaml @@ -0,0 +1,3 @@ +test: + bad: "" + fail: ""