From 554b8544f5118c1f0e977504302a2f12822262d4 Mon Sep 17 00:00:00 2001 From: "alexey.zh" Date: Sun, 5 Jan 2025 14:18:46 +0500 Subject: [PATCH] REFACTORING-V2 - add rawargs-v2. refactored version, more tests. --- pkg/cmd/rawargs.go | 165 +++++++++--------- pkg/cmd/rawargs_test.go | 360 +++++++++++++++++++++++----------------- 2 files changed, 278 insertions(+), 247 deletions(-) diff --git a/pkg/cmd/rawargs.go b/pkg/cmd/rawargs.go index 18f526f..f4e2ad4 100644 --- a/pkg/cmd/rawargs.go +++ b/pkg/cmd/rawargs.go @@ -21,12 +21,9 @@ type CmdArgsRawRecognized struct { HasStdin bool } -func allEmpty(where []string) bool { - if len(where) == 0 { - return true - } - for _, s := range where { - if strings.TrimSpace(s) != "" { +func allEmpty(values []string) bool { + for _, v := range values { + if strings.TrimSpace(v) != "" { return false } } @@ -41,138 +38,124 @@ func ParseArgs() (CmdArgsRawRecognized, error) { arg := args[i] switch { - // --filename=pod.yaml - case strings.HasPrefix(arg, "--filename="): - filenameGiven := strings.TrimPrefix(arg, "--filename=") - if filenameGiven == "" { - return result, fmt.Errorf("missing value for flag %s", arg) - } - err := handleStdin(filenameGiven, &result) - if err != nil { + // Handle --filename= or -f= + case strings.HasPrefix(arg, "--filename="), strings.HasPrefix(arg, "-f="): + if err := handleFilename(strings.SplitN(arg, "=", 2)[1], &result); err != nil { return result, err } - if filenameGiven != "-" { - result.Filenames = append(result.Filenames, filenameGiven) - } - // -f=pod.yaml - case strings.HasPrefix(arg, "-f="): - filenameGiven := strings.TrimPrefix(arg, "-f=") - if filenameGiven == "" { + // Handle --filename or -f with a separate value + case arg == "--filename" || arg == "-f": + if i+1 >= len(args) || args[i+1] == "" { return result, fmt.Errorf("missing value for flag %s", arg) } - err := handleStdin(filenameGiven, &result) - if err != nil { + if err := handleFilename(args[i+1], &result); err != nil { return result, err } - if filenameGiven != "-" { - result.Filenames = append(result.Filenames, filenameGiven) - } - // --filename pod.yaml -f pod.yaml - case arg == "--filename" || arg == "-f": - if i+1 < len(args) { - filenameGiven := args[i+1] - if filenameGiven == "" { - return result, fmt.Errorf("missing value for flag %s", arg) - } - err := handleStdin(filenameGiven, &result) - if err != nil { - return result, err - } - if filenameGiven != "-" { - result.Filenames = append(result.Filenames, filenameGiven) - } - i++ // Skip the next argument since it's the value - } else { - return result, fmt.Errorf("missing value for flag %s", arg) - } + i++ // Skip the next argument - // --envsubst-allowed-vars=HOME,USER + // Handle --envsubst-allowed-vars= case strings.HasPrefix(arg, "--envsubst-allowed-vars="): - split := strings.Split(strings.TrimPrefix(arg, "--envsubst-allowed-vars="), ",") - if allEmpty(split) { - return result, fmt.Errorf("missing value for flag %s", arg) + list, err := appendList(strings.TrimPrefix(arg, "--envsubst-allowed-vars=")) + if err != nil { + return result, err } - result.EnvsubstAllowedVars = append(result.EnvsubstAllowedVars, split...) + result.EnvsubstAllowedVars = append(result.EnvsubstAllowedVars, list...) - // --envsubst-allowed-vars HOME,USER + // Handle --envsubst-allowed-vars with a separate value case arg == "--envsubst-allowed-vars": - if i+1 < len(args) { - split := strings.Split(args[i+1], ",") - if allEmpty(split) { - return result, fmt.Errorf("missing value for flag %s", arg) - } - result.EnvsubstAllowedVars = append(result.EnvsubstAllowedVars, split...) - i++ // Skip the next argument since it's the value - } else { + if i+1 >= len(args) || args[i+1] == "" { return result, fmt.Errorf("missing value for flag %s", arg) } + list, err := appendList(args[i+1]) + if err != nil { + return result, err + } + result.EnvsubstAllowedVars = append(result.EnvsubstAllowedVars, list...) + i++ // Skip the next argument - // --envsubst-allowed-prefixes=CI_,APP_ + // Handle --envsubst-allowed-prefixes= case strings.HasPrefix(arg, "--envsubst-allowed-prefixes="): - split := strings.Split(strings.TrimPrefix(arg, "--envsubst-allowed-prefixes="), ",") - if allEmpty(split) { - return result, fmt.Errorf("missing value for flag %s", arg) + list, err := appendList(strings.TrimPrefix(arg, "--envsubst-allowed-prefixes=")) + if err != nil { + return result, err } - result.EnvsubstAllowedPrefix = append(result.EnvsubstAllowedPrefix, split...) + result.EnvsubstAllowedPrefix = append(result.EnvsubstAllowedPrefix, list...) - // --envsubst-allowed-prefixes CI_,APP_ + // Handle --envsubst-allowed-prefixes with a separate value case arg == "--envsubst-allowed-prefixes": - if i+1 < len(args) { - split := strings.Split(args[i+1], ",") - if allEmpty(split) { - return result, fmt.Errorf("missing value for flag %s", arg) - } - result.EnvsubstAllowedPrefix = append(result.EnvsubstAllowedPrefix, split...) - i++ // Skip the next argument since it's the value - } else { + if i+1 >= len(args) || args[i+1] == "" { return result, fmt.Errorf("missing value for flag %s", arg) } + list, err := appendList(args[i+1]) + if err != nil { + return result, err + } + result.EnvsubstAllowedPrefix = append(result.EnvsubstAllowedPrefix, list...) + i++ // Skip the next argument - // --recursive, -R + // Handle recursive and help flags case arg == "--recursive" || arg == "-R": result.Recursive = true - // --help, -h case arg == "--help" || arg == "-h": result.Help = true + // Handle unrecognized arguments default: result.Others = append(result.Others, arg) } } - // trying to get allowed-vars config from envs + // Load allowed vars and prefixes from environment variables if len(result.EnvsubstAllowedVars) == 0 { - if value, exists := os.LookupEnv(envsubstAllowedVarsEnv); exists { - split := strings.Split(value, ",") - if allEmpty(split) { - return result, fmt.Errorf("missing value for env: %s", envsubstAllowedVarsEnv) - } - result.EnvsubstAllowedVars = split + if err := loadEnvVars(envsubstAllowedVarsEnv, &result.EnvsubstAllowedVars); err != nil { + return result, err } } - - // trying to get allowed-prefixes from envs if len(result.EnvsubstAllowedPrefix) == 0 { - if value, exists := os.LookupEnv(envsubstAllowedPrefixesEnv); exists { - split := strings.Split(value, ",") - if allEmpty(split) { - return result, fmt.Errorf("missing value for env: %s", envsubstAllowedPrefixesEnv) - } - result.EnvsubstAllowedPrefix = split + if err := loadEnvVars(envsubstAllowedPrefixesEnv, &result.EnvsubstAllowedPrefix); err != nil { + return result, err } } return result, nil } -func handleStdin(filenameGiven string, result *CmdArgsRawRecognized) error { - if filenameGiven == "-" { +func handleFilename(filename string, result *CmdArgsRawRecognized) error { + if filename == "" { + return fmt.Errorf("missing filename value") + } + if filename == "-" { if result.HasStdin { return fmt.Errorf("multiple redirection to stdin detected") } result.HasStdin = true + } else { + result.Filenames = append(result.Filenames, filename) + } + return nil +} + +func appendList(value string) ([]string, error) { + split := strings.Split(value, ",") + if value == "" || allEmpty(split) { + return nil, fmt.Errorf("empty list value") } + return split, nil +} + +func loadEnvVars(envKey string, target *[]string) error { + value, exists := os.LookupEnv(envKey) + if !exists { + return nil + } + + split := strings.Split(value, ",") + if allEmpty(split) { + return fmt.Errorf("missing value for env: %s", envKey) + } + + *target = split return nil } diff --git a/pkg/cmd/rawargs_test.go b/pkg/cmd/rawargs_test.go index aecb014..4f10a8d 100644 --- a/pkg/cmd/rawargs_test.go +++ b/pkg/cmd/rawargs_test.go @@ -3,7 +3,6 @@ package cmd import ( "os" "reflect" - "strings" "testing" ) @@ -169,7 +168,7 @@ func TestParseArgs(t *testing.T) { expectedError: false, }, { - name: "Valid args with unrecognized argument that looks like a short flag", + name: "Valid args with unrecognized argument that looks like a short flag (1)", args: []string{"-R", "-xyz"}, expectedResult: CmdArgsRawRecognized{ Recursive: true, @@ -177,6 +176,15 @@ func TestParseArgs(t *testing.T) { }, expectedError: false, }, + { + name: "Valid args with unrecognized argument that looks like a short flag (2)", + args: []string{"-h", "-xyz"}, + expectedResult: CmdArgsRawRecognized{ + Help: true, + Others: []string{"-xyz"}, + }, + expectedError: false, + }, } for _, tc := range cases { @@ -198,105 +206,6 @@ func TestParseArgs(t *testing.T) { } } -func TestParseArgs_EnvFallback(t *testing.T) { - os.Setenv("ENVSUBST_ALLOWED_VARS", "HOME,USER") - os.Setenv("ENVSUBST_ALLOWED_PREFIXES", "APP_,CI_") - defer os.Unsetenv("ENVSUBST_ALLOWED_VARS") - defer os.Unsetenv("ENVSUBST_ALLOWED_PREFIXES") - - os.Args = []string{"cmd"} - result, err := ParseArgs() - - if err != nil { - t.Fatalf("Unexpected error: %v", err) - } - - expectedVars := []string{"HOME", "USER"} - expectedPrefixes := []string{"APP_", "CI_"} - if len(result.EnvsubstAllowedVars) != len(expectedVars) { - t.Errorf("Expected allowed vars %v, got %v", expectedVars, result.EnvsubstAllowedVars) - } - for i, varName := range expectedVars { - if result.EnvsubstAllowedVars[i] != varName { - t.Errorf("Expected allowed var %s, got %s", varName, result.EnvsubstAllowedVars[i]) - } - } - if len(result.EnvsubstAllowedPrefix) != len(expectedPrefixes) { - t.Errorf("Expected allowed prefixes %v, got %v", expectedPrefixes, result.EnvsubstAllowedPrefix) - } - for i, prefix := range expectedPrefixes { - if result.EnvsubstAllowedPrefix[i] != prefix { - t.Errorf("Expected allowed prefix %s, got %s", prefix, result.EnvsubstAllowedPrefix[i]) - } - } -} - -func TestParseArgs_CmdAndEnvFlags(t *testing.T) { - // Set environment variables - os.Setenv("ENVSUBST_ALLOWED_VARS", "HOME,USER") - os.Setenv("ENVSUBST_ALLOWED_PREFIXES", "APP_,CI_") - defer os.Unsetenv("ENVSUBST_ALLOWED_VARS") - defer os.Unsetenv("ENVSUBST_ALLOWED_PREFIXES") - - // Set command-line arguments - os.Args = []string{ - "cmd", - "--envsubst-allowed-vars=CMD_VAR1,CMD_VAR2", - "--envsubst-allowed-prefixes=CMD_", - } - - result, err := ParseArgs() - - if err != nil { - t.Fatalf("Unexpected error: %v", err) - } - - // Verify that command-line flags take precedence over environment variables - expectedVars := []string{"CMD_VAR1", "CMD_VAR2"} - expectedPrefixes := []string{"CMD_"} - - if len(result.EnvsubstAllowedVars) != len(expectedVars) { - t.Errorf("Expected allowed vars %v, got %v", expectedVars, result.EnvsubstAllowedVars) - } - for i, varName := range expectedVars { - if result.EnvsubstAllowedVars[i] != varName { - t.Errorf("Expected allowed var %s, got %s", varName, result.EnvsubstAllowedVars[i]) - } - } - - if len(result.EnvsubstAllowedPrefix) != len(expectedPrefixes) { - t.Errorf("Expected allowed prefixes %v, got %v", expectedPrefixes, result.EnvsubstAllowedPrefix) - } - for i, prefix := range expectedPrefixes { - if result.EnvsubstAllowedPrefix[i] != prefix { - t.Errorf("Expected allowed prefix %s, got %s", prefix, result.EnvsubstAllowedPrefix[i]) - } - } -} - -func TestParseArgs_EmptyEnvVars(t *testing.T) { - // Set empty environment variables - os.Setenv("ENVSUBST_ALLOWED_VARS", "") - os.Setenv("ENVSUBST_ALLOWED_PREFIXES", "") - defer os.Unsetenv("ENVSUBST_ALLOWED_VARS") - defer os.Unsetenv("ENVSUBST_ALLOWED_PREFIXES") - - os.Args = []string{"cmd"} - _, err := ParseArgs() - - // Expect an error due to empty environment variables - if err == nil { - t.Fatal("Expected an error for empty environment variables, but got none") - } - - expectedErrorVars := "missing value for env: ENVSUBST_ALLOWED_VARS" - expectedErrorPrefixes := "missing value for env: ENVSUBST_ALLOWED_PREFIXES" - - if !strings.Contains(err.Error(), expectedErrorVars) && !strings.Contains(err.Error(), expectedErrorPrefixes) { - t.Errorf("Expected error to mention missing values for ENVSUBST_ALLOWED_VARS or ENVSUBST_ALLOWED_PREFIXES, got '%s'", err.Error()) - } -} - func TestParseArgs_SingleStdin(t *testing.T) { os.Args = []string{"cmd", "--filename", "-"} result, err := ParseArgs() @@ -328,74 +237,213 @@ func TestParseArgs_MultipleStdin(t *testing.T) { } } -func TestParseArgs_EmptyFilename(t *testing.T) { - os.Args = []string{"cmd", "--filename="} - _, err := ParseArgs() - - if err == nil { - t.Fatal("Expected an error for empty filename, but got none") - } - - expectedError := "missing value for flag --filename=" - if err.Error() != expectedError { - t.Errorf("Expected error '%s', got '%s'", expectedError, err.Error()) +func TestLoadEnvVars(t *testing.T) { + tests := []struct { + name string + envKey string + envValue string + initial []string + expected []string + expectErr bool + }{ + { + name: "Valid Environment Variable", + envKey: "TEST_ENV", + envValue: "VAR1,VAR2,VAR3", + initial: nil, + expected: []string{"VAR1", "VAR2", "VAR3"}, + expectErr: false, + }, + { + name: "Missing Environment Variable", + envKey: "MISSING_ENV", + envValue: "", + initial: nil, + expected: nil, + expectErr: false, + }, + // not an error, it means that configuration ENVS were not set + // an error will be handled in subst stage + { + name: "Empty Environment Variable Value", + envKey: "EMPTY_ENV", + envValue: "", + initial: nil, + expected: nil, + expectErr: false, + }, + { + name: "Whitespace Only Environment Variable", + envKey: "WHITESPACE_ENV", + envValue: " , , ", + initial: nil, + expected: nil, + expectErr: true, + }, + { + name: "Pre-existing Values in Target", + envKey: "TEST_ENV", + envValue: "NEW1,NEW2", + initial: []string{"OLD1", "OLD2"}, + expected: []string{"NEW1", "NEW2"}, + expectErr: false, + }, } -} -func TestParseArgs_FilenamesProvided(t *testing.T) { - os.Args = []string{"cmd", "--filename=pod.yaml", "-f=config.yaml", "--filename", "app.yaml"} - result, err := ParseArgs() + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + // Setup environment variable + if test.envValue != "" { + os.Setenv(test.envKey, test.envValue) + defer os.Unsetenv(test.envKey) + } - if err != nil { - t.Fatalf("Unexpected error: %v", err) - } + // Initialize target + target := test.initial + + // Call function + err := loadEnvVars(test.envKey, &target) + + // Check for errors + if test.expectErr { + if err == nil { + t.Errorf("Test '%s': expected error but got none", test.name) + } + } else { + if err != nil { + t.Errorf("Test '%s': unexpected error: %v", test.name, err) + } + } - expectedFilenames := []string{"pod.yaml", "config.yaml", "app.yaml"} - if len(result.Filenames) != len(expectedFilenames) { - t.Errorf("Expected filenames %v, got %v", expectedFilenames, result.Filenames) - } - for i, filename := range expectedFilenames { - if result.Filenames[i] != filename { - t.Errorf("Expected filename %s, got %s", filename, result.Filenames[i]) - } + // Check result + if !reflect.DeepEqual(target, test.expected) { + t.Errorf("Test '%s': result = %v; want %v", test.name, target, test.expected) + } + }) } } -func TestParseArgs_NoFilenames(t *testing.T) { - os.Args = []string{"cmd"} - result, err := ParseArgs() - - if err != nil { - t.Fatalf("Unexpected error: %v", err) +func TestParseArgs_ErrorsAndReturns(t *testing.T) { + tests := []struct { + name string + args []string + envVars map[string]string + expectErr string + validate func(t *testing.T, result CmdArgsRawRecognized) + }{ + { + name: "Missing value for --filename=", + args: []string{"app", "--filename="}, + expectErr: "missing filename value", + }, + { + name: "Missing value for --filename", + args: []string{"app", "--filename"}, + expectErr: "missing value for flag --filename", + }, + { + name: "Multiple redirection to stdin", + args: []string{"app", "--filename=-", "--filename=-"}, + expectErr: "multiple redirection to stdin detected", + }, + { + name: "Empty list value for --envsubst-allowed-vars=", + args: []string{"app", "--envsubst-allowed-vars="}, + expectErr: "empty list value", + }, + { + name: "Empty list value for --envsubst-allowed-vars", + args: []string{"app", "--envsubst-allowed-vars"}, + expectErr: "missing value for flag --envsubst-allowed-vars", + }, + { + name: "Empty list value for --envsubst-allowed-prefixes=", + args: []string{"app", "--envsubst-allowed-prefixes="}, + expectErr: "empty list value", + }, + { + name: "Empty list value for --envsubst-allowed-prefixes", + args: []string{"app", "--envsubst-allowed-prefixes"}, + expectErr: "missing value for flag --envsubst-allowed-prefixes", + }, + { + name: "Empty environment variable for ENVSUBST_ALLOWED_VARS", + args: []string{"app"}, + envVars: map[string]string{ + "ENVSUBST_ALLOWED_VARS": "", + }, + expectErr: "missing value for env: ENVSUBST_ALLOWED_VARS", + }, + { + name: "Empty environment variable for ENVSUBST_ALLOWED_PREFIXES", + args: []string{"app"}, + envVars: map[string]string{ + "ENVSUBST_ALLOWED_PREFIXES": "", + }, + expectErr: "missing value for env: ENVSUBST_ALLOWED_PREFIXES", + }, + { + name: "Successful parsing with all flags", + args: []string{"app", "--filename=test.yaml", "--envsubst-allowed-vars=VAR1,VAR2", "--envsubst-allowed-prefixes=PREFIX1,PREFIX2", "--recursive", "--help"}, + validate: func(t *testing.T, result CmdArgsRawRecognized) { + if len(result.Filenames) != 1 || result.Filenames[0] != "test.yaml" { + t.Errorf("Expected filename 'test.yaml', got %v", result.Filenames) + } + if len(result.EnvsubstAllowedVars) != 2 || result.EnvsubstAllowedVars[0] != "VAR1" || result.EnvsubstAllowedVars[1] != "VAR2" { + t.Errorf("Expected EnvsubstAllowedVars to contain [VAR1, VAR2], got %v", result.EnvsubstAllowedVars) + } + if len(result.EnvsubstAllowedPrefix) != 2 || result.EnvsubstAllowedPrefix[0] != "PREFIX1" || result.EnvsubstAllowedPrefix[1] != "PREFIX2" { + t.Errorf("Expected EnvsubstAllowedPrefix to contain [PREFIX1, PREFIX2], got %v", result.EnvsubstAllowedPrefix) + } + if !result.Recursive { + t.Errorf("Expected Recursive to be true") + } + if !result.Help { + t.Errorf("Expected Help to be true") + } + }, + }, } - if len(result.Filenames) != 0 { - t.Errorf("Expected Filenames to be empty, got: %v", result.Filenames) + originalEnv := make(map[string]string) + for _, k := range os.Environ() { + originalEnv[k] = os.Getenv(k) } - if result.HasStdin { - t.Errorf("Expected HasStdin to be false") - } -} + defer func() { + for k, v := range originalEnv { + os.Setenv(k, v) + } + }() -func TestParseArgs_StdinAndFile(t *testing.T) { - os.Args = []string{"cmd", "--filename=-", "-f=pod.yaml"} - result, err := ParseArgs() + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + // Set environment variables for the test + for k, v := range test.envVars { + os.Setenv(k, v) + } + defer func(envVars map[string]string) { + for k := range envVars { + os.Unsetenv(k) + } + }(test.envVars) - if err != nil { - t.Fatalf("Unexpected error: %v", err) - } + // Simulate command-line arguments + os.Args = test.args - if !result.HasStdin { - t.Errorf("Expected HasStdin to be true") - } + // Run ParseArgs + result, err := ParseArgs() + if test.expectErr != "" { + if err == nil || err.Error() != test.expectErr { + t.Fatalf("Expected error '%s', got '%v'", test.expectErr, err) + } + } else if err != nil { + t.Fatalf("Unexpected error: %v", err) + } - expectedFilenames := []string{"pod.yaml"} - if len(result.Filenames) != len(expectedFilenames) { - t.Errorf("Expected filenames %v, got %v", expectedFilenames, result.Filenames) - } - for i, filename := range expectedFilenames { - if result.Filenames[i] != filename { - t.Errorf("Expected filename %s, got %s", filename, result.Filenames[i]) - } + // Validate results + if test.validate != nil { + test.validate(t, result) + } + }) } }