[#1587] Working general CLI parser

backend/appcfg/stork/cli.go

@@ -21,15 +21,10 @@ type environmentFileSettings struct {
 // Read hook directory settings. They are parsed after environment file
 // settings but before the main settings.
 // It allows us to merge the hook flags with the core flags into a single output.
-type hookDirectorySettings struct {
+type HookDirectorySettings struct {
 	HookDirectory string `long:"hook-directory" description:"The path to the hook directory" env:"STORK_@APP_HOOK_DIRECTORY" default:"/usr/lib/stork-@APP/hooks"`
 }
 
-type BaseSettings struct {
-	environmentFileSettings
-	hookDirectorySettings
-}
-
 // Defines the type for set of hook settings grouped by the hook name.
 type GroupedHookCLIFlags map[string]hooks.HookSettings
 
@@ -42,6 +37,11 @@ type CLIParser struct {
 
 // Constructs CLI parser.
 func NewCLIParser(parser *flags.Parser, app string, onLoadEnvironmentFileCallback func()) *CLIParser {
+	if app != "server" && app != "agent" {
+		// Programming error.
+		panic("invalid application name")
+	}
+
 	return &CLIParser{
 		parser:                        parser,
 		application:                   strings.ToLower(app),
@@ -51,23 +51,25 @@ func NewCLIParser(parser *flags.Parser, app string, onLoadEnvironmentFileCallbac
 
 // Parse the command line arguments into Stork-specific GO structures.
 // At the end, it composes the CLI parser from all the flags and runs it.
-func (p *CLIParser) Parse() (hookFlags GroupedHookCLIFlags, isHelp bool, err error) {
-	allHookFLags, err := p.bootstrap()
+// Returns a hook directory settings, hook settings extracted from the hooks,
+// flag indication if the help was requested and an error if any.
+func (p *CLIParser) Parse() (*HookDirectorySettings, GroupedHookCLIFlags, bool, error) {
+	hookDirectorySettings, allHookFLags, err := p.bootstrap()
 	if err != nil {
 		if isHelpRequest(err) {
-			return nil, true, nil
+			return nil, nil, true, nil
 		}
-		return nil, false, err
+		return nil, nil, false, err
 	}
 
 	err = p.parse()
 	if err != nil {
 		if isHelpRequest(err) {
-			return nil, true, nil
+			return nil, nil, true, nil
 		}
-		return nil, false, err
+		return nil, nil, false, err
 	}
-	return allHookFLags, false, nil
+	return hookDirectorySettings, allHookFLags, false, nil
 }
 
 // Parse the CLI flags stored in the main parser.
@@ -85,35 +87,50 @@ func (p *CLIParser) parse() (err error) {
 // is provided, the content is loaded.
 // Next, it parses the hooks location and extracts their CLI flags.
 // The hook flags are then merged with the core flags.
-func (p *CLIParser) bootstrap() (GroupedHookCLIFlags, error) {
+func (p *CLIParser) bootstrap() (*HookDirectorySettings, GroupedHookCLIFlags, error) {
 	// Environment variables.
 	envFileSettings := &environmentFileSettings{}
 	envParser := p.createSubParser(envFileSettings)
 	if _, err := envParser.Parse(); err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 	err := p.loadEnvironmentFile(envFileSettings)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 
 	// Process the hook directory location.
-	hookDirectorySettings := &hookDirectorySettings{}
+	hookDirectorySettings := &HookDirectorySettings{}
 	hookParser := p.createSubParser(hookDirectorySettings)
 	if _, err := hookParser.Parse(); err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 
 	allHookCLIFlags, err := p.collectHookCLIFlags(hookDirectorySettings)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 	err = p.mergeHookFlags(allHookCLIFlags)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
+	}
+
+	// Append the parser-related flags to the main parser.
+	group, err := p.parser.AddGroup("Environment File Flags", "", envFileSettings)
+	if err != nil {
+		err = errors.Wrap(err, "cannot add the environment file group")
+		return nil, nil, err
 	}
+	p.substitutePlaceholdersInGroup(group)
 
-	return allHookCLIFlags, nil
+	group, err = p.parser.AddGroup("Hook Directory Flags", "", hookDirectorySettings)
+	if err != nil {
+		err = errors.Wrap(err, "cannot add the hook directory group")
+		return nil, nil, err
+	}
+	p.substitutePlaceholdersInGroup(group)
+
+	return hookDirectorySettings, allHookCLIFlags, nil
 }
 
 // Merges the CLI flags of the hooks with the core CLI flags.
@@ -192,20 +209,26 @@ func (p *CLIParser) createSubParser(settings any) *flags.Parser {
 // Substitutes the placeholders in the defaults and environment variable names.
 func (p *CLIParser) substitutePlaceholders(parser *flags.Parser) {
 	for _, group := range parser.Groups() {
-		for _, option := range group.Options() {
-			// Defaults.
-			for i, d := range option.Default {
-				option.Default[i] = strings.Replace(d, "@APP", p.application, 1)
-			}
-
-			// Environment variables.
-			option.EnvDefaultKey = strings.Replace(
-				option.EnvDefaultKey,
-				"@APP",
-				strings.ToUpper(p.application),
-				1,
-			)
+		p.substitutePlaceholdersInGroup(group)
+	}
+}
+
+// Substitutes the placeholders in the defaults and environment variable names
+// in a group of flags.
+func (p *CLIParser) substitutePlaceholdersInGroup(group *flags.Group) {
+	for _, option := range group.Options() {
+		// Defaults.
+		for i, d := range option.Default {
+			option.Default[i] = strings.Replace(d, "@APP", p.application, 1)
 		}
+
+		// Environment variables.
+		option.EnvDefaultKey = strings.Replace(
+			option.EnvDefaultKey,
+			"@APP",
+			strings.ToUpper(p.application),
+			1,
+		)
 	}
 }
 
@@ -234,15 +257,20 @@ func (p *CLIParser) loadEnvironmentFile(envFileSettings *environmentFileSettings
 }
 
 // Extracts the CLI flags from the hooks.
-func (p *CLIParser) collectHookCLIFlags(hookDirectorySettings *hookDirectorySettings) (map[string]hooks.HookSettings, error) {
+func (p *CLIParser) collectHookCLIFlags(hookDirectorySettings *HookDirectorySettings) (map[string]hooks.HookSettings, error) {
 	allCLIFlags := map[string]hooks.HookSettings{}
 	stat, err := os.Stat(hookDirectorySettings.HookDirectory)
 	switch {
 	case err == nil && stat.IsDir():
 		// Gather the hook flags.
 		hookWalker := hooksutil.NewHookWalker()
+		program := hooks.HookProgramServer
+		if p.application == "agent" {
+			program = hooks.HookProgramAgent
+		}
+
 		allCLIFlags, err = hookWalker.CollectCLIFlags(
-			hooks.HookProgramServer,
+			program,
 			hookDirectorySettings.HookDirectory,
 		)
 		if err != nil {

backend/appcfg/stork/cli_test.go

@@ -46,7 +46,6 @@ func TestEnvironmentFileIsLoaded(t *testing.T) {
 	}
 
 	type settings struct {
-		BaseSettings
 		DBHost   string `long:"db-host" description:"The host name, IP address or socket where database is available" env:"STORK_DATABASE_HOST" default:""`
 		RESTHost string `long:"rest-host" description:"The IP to listen on" default:"" env:"STORK_REST_HOST"`
 	}
@@ -58,16 +57,15 @@ func TestEnvironmentFileIsLoaded(t *testing.T) {
 	parser := NewCLIParser(flagParser, "server", func() {})
 
 	// Act
-	hookFlags, isHelp, err := parser.Parse()
+	hookDirSettings, hookFlags, isHelp, err := parser.Parse()
 
 	// Assert
 	require.NoError(t, err)
 	require.False(t, isHelp)
 	require.Empty(t, hookFlags)
 
 	require.Equal(t, "foo", data.DBHost)
-	// TODO: Fix this test
-	// require.Equal(t, "bar", data.GeneralSettings.HookDirectory)
+	require.Equal(t, "bar", hookDirSettings.HookDirectory)
 	require.Equal(t, "baz", data.RESTHost)
 }
 
@@ -95,12 +93,13 @@ func TestEnvironmentFileIsInvalid(t *testing.T) {
 	parser := NewCLIParser(flagParser, "server", func() {})
 
 	// Act
-	hookSettings, isHelp, err := parser.Parse()
+	hookDirSettings, hookSettings, isHelp, err := parser.Parse()
 
 	// Assert
 	require.Error(t, err)
 	require.False(t, isHelp)
 	require.Empty(t, hookSettings)
+	require.Nil(t, hookDirSettings)
 }
 
 // Test that the CLI arguments take precedence over the environment file and
@@ -133,7 +132,6 @@ func TestParseArgsFromMultipleSources(t *testing.T) {
 	}
 
 	type settings struct {
-		BaseSettings
 		DBHost   string `long:"db-host" description:"The host name, IP address or socket where database is available" env:"STORK_DATABASE_HOST" default:""`
 		RESTHost string `long:"rest-host" description:"The IP to listen on" default:"" env:"STORK_REST_HOST"`
 		TLSCert  string `long:"rest-tls-certificate" description:"The path to the TLS certificate" env:"STORK_REST_TLS_CERTIFICATE" default:""`
@@ -143,12 +141,14 @@ func TestParseArgsFromMultipleSources(t *testing.T) {
 
 	parser := NewCLIParser(flags.NewParser(data, flags.Default), "server", func() {})
 	// Act
-	hookSettings, isHelp, err := parser.Parse()
+	hookDirSettings, hookSettings, isHelp, err := parser.Parse()
 
 	// Assert
 	require.NoError(t, err)
 	require.False(t, isHelp)
 	require.Empty(t, hookSettings)
+	require.NotNil(t, hookDirSettings)
+	require.Equal(t, "/usr/lib/stork-server/hooks", hookDirSettings.HookDirectory)
 	require.EqualValues(t, "database-host-envvar", data.DBHost)
 	require.EqualValues(t, "rest-host-envfile", data.RESTHost)
 	require.EqualValues(t, "certificate-envfile", data.TLSCert)
@@ -168,12 +168,13 @@ func TestCLIParserRejectsWrongCLIArguments(t *testing.T) {
 	parser := NewCLIParser(flags.NewParser(data, flags.Default), "server", func() {})
 
 	// Act
-	hookSettings, isHelp, err := parser.Parse()
+	hookDirSettings, hookSettings, isHelp, err := parser.Parse()
 
 	// Assert
 	require.Error(t, err)
 	require.False(t, isHelp)
 	require.Empty(t, hookSettings)
+	require.Nil(t, hookDirSettings)
 }
 
 // Test that the namespaces are correct.
@@ -246,12 +247,13 @@ func TestCollectHookCLIFlagsForNonDirectoryPath(t *testing.T) {
 
 	// Act
 	os.Args = []string{"stork-server", "--hook-directory", path}
-	hookFlags, isHelp, err := parser.Parse()
+	hookDirSettings, hookFlags, isHelp, err := parser.Parse()
 
 	// Assert
 	require.ErrorContains(t, err, "hook directory path is not pointing to a directory")
 	require.False(t, isHelp)
 	require.Empty(t, hookFlags)
+	require.Nil(t, hookDirSettings)
 }
 
 // Test that the no error is returned if the hook directory doesn't exist.
@@ -260,7 +262,7 @@ func TestCollectHookCLIFlagsForMissingDirectory(t *testing.T) {
 	sb := testutil.NewSandbox()
 	defer sb.Close()
 	parser := NewCLIParser(nil, "server", func() {})
-	hookSettings := &hookDirectorySettings{
+	hookSettings := &HookDirectorySettings{
 		path.Join(sb.BasePath, "non-exists-directory"),
 	}
 
@@ -295,10 +297,13 @@ func TestParseHookSettingsFromEnvironmentVariables(t *testing.T) {
 	parser := NewCLIParser(flags.NewParser(data, flags.Default), "server", func() {})
 
 	// Act
-	err := parser.parse(hookFlags)
+	mergeErr := parser.mergeHookFlags(hookFlags)
+	parseErr := parser.parse()
 
 	// Assert
-	require.NoError(t, err)
+	require.NoError(t, mergeErr)
+	require.NoError(t, parseErr)
+
 	// ToDO: Fix this test
 	// require.Contains(t, settings.HooksSettings, "baz")
 	require.Equal(t, "fooBar", hookFlags["baz"].(*hookSettings).FooBar)
@@ -325,10 +330,12 @@ func TestParseHookSettingsFromCLI(t *testing.T) {
 	parser := NewCLIParser(flags.NewParser(data, flags.Default), "server", func() {})
 
 	// Act
-	err := parser.parse(hookFlags)
+	mergeErr := parser.mergeHookFlags(hookFlags)
+	parseErr := parser.parse()
 
 	// Assert
-	require.NoError(t, err)
+	require.NoError(t, mergeErr)
+	require.NoError(t, parseErr)
 	// TODO: Fix this test
 	// require.Contains(t, settings.HooksSettings, "baz")
 	require.Equal(t, "fooBar", hookFlags["baz"].(*hookSettings).FooBar)
@@ -357,8 +364,75 @@ func TestPaseHookSettingsDuplicatedNamespace(t *testing.T) {
 	parser := NewCLIParser(flags.NewParser(data, flags.Default), "server", func() {})
 
 	// Act
-	err := parser.parse(hookFlags)
+	err := parser.mergeHookFlags(hookFlags)
 
 	// Assert
 	require.ErrorContains(t, err, "two hooks using the same configuration namespace")
 }
+
+// Test that the help is properly printed and it includes the hook settings.
+func TestParseHelp(t *testing.T) {
+	// Arrange
+	defer testutil.CreateOsArgsRestorePoint()()
+	os.Args = []string{
+		"program-name",
+		"--help",
+	}
+
+	type hookSettings struct {
+		FooBar string `long:"foo-bar" description:"Lorem ipsum" env:"FOO_BAR"`
+	}
+
+	hookFlags := map[string]hooks.HookSettings{
+		"baz": &hookSettings{},
+	}
+
+	type settings struct {
+		TLSCert string `long:"tls-cert" env:"TLS_CERT" description:"The path to the TLS certificate"`
+	}
+	data := &settings{}
+
+	parser := NewCLIParser(flags.NewParser(data, flags.Default), "server", func() {})
+	_ = parser.mergeHookFlags(hookFlags)
+
+	// Act
+	var isHelp bool
+	var err error
+	stdout, stderr, captureErr := testutil.CaptureOutput(func() {
+		_, _, isHelp, err = parser.Parse()
+	})
+
+	// Assert
+	require.NoError(t, err)
+	require.NoError(t, captureErr)
+	require.True(t, isHelp)
+	require.Empty(t, stderr)
+
+	expectedHelp :=
+		`Usage:
+  program-name [OPTIONS]
+
+Application Options:
+      --tls-cert=       The path to the TLS certificate [$TLS_CERT]
+
+Hook 'baz' Flags:
+      --baz.foo-bar=    Lorem ipsum [$STORK_SERVER_HOOK_BAZ_FOO_BAR]
+
+Environment File Flags:
+      --env-file=       Environment file location; applicable only if the
+                        use-env-file is provided (default:
+                        /etc/stork/server.env)
+      --use-env-file    Read the environment variables from the environment file
+
+Hook Directory Flags:
+      --hook-directory= The path to the hook directory (default:
+                        /usr/lib/stork-server/hooks)
+                        [$STORK_SERVER_HOOK_DIRECTORY]
+
+Help Options:
+  -h, --help            Show this help message
+
+`
+
+	require.Equal(t, expectedHelp, string(stdout))
+}