Merge pull request #34 from ncode/fix/required-flag-validation

Refine required flag validation

Author: ncode

cmd/cleanup.go

@@ -44,6 +44,10 @@ var cleanupCmd = &cobra.Command{
 		}
 
 		serviceID := cmd.InheritedFlags().Lookup("service-id").Value.String()
+		if serviceID == "" {
+			logger.Error("Service ID is required")
+			return fmt.Errorf("service-id is required")
+		}
 		tagPrefix := cmd.InheritedFlags().Lookup("tag-prefix").Value.String()
 
 		t := tagit.New(

cmd/cleanup_test.go

@@ -22,13 +22,7 @@ func TestCleanupCmd(t *testing.T) {
 			name:          "Missing required service-id",
 			args:          []string{"cleanup"},
 			expectError:   true,
-			errorContains: "required flag(s)",
-		},
-		{
-			name:          "Missing required script (even though not used for cleanup)",
-			args:          []string{"cleanup", "--service-id=test-service"},
-			expectError:   true,
-			errorContains: "required flag(s) \"script\" not set",
+			errorContains: "service-id is required",
 		},
 	}
 
@@ -38,9 +32,7 @@ func TestCleanupCmd(t *testing.T) {
 			cmd := &cobra.Command{Use: "tagit"}
 			cmd.PersistentFlags().StringP("consul-addr", "c", "127.0.0.1:8500", "consul address")
 			cmd.PersistentFlags().StringP("service-id", "s", "", "consul service id")
-			cmd.MarkPersistentFlagRequired("service-id")
 			cmd.PersistentFlags().StringP("script", "x", "", "path to script used to generate tags")
-			cmd.MarkPersistentFlagRequired("script")
 			cmd.PersistentFlags().StringP("tag-prefix", "p", "tagged", "prefix to be added to tags")
 			cmd.PersistentFlags().StringP("interval", "i", "60s", "interval to run the script")
 			cmd.PersistentFlags().StringP("token", "t", "", "consul token")
@@ -63,8 +55,7 @@ func TestCleanupCmd(t *testing.T) {
 			if tt.expectError {
 				assert.Error(t, err)
 				if tt.errorContains != "" {
-					output := buf.String()
-					assert.Contains(t, output, tt.errorContains)
+					assert.Contains(t, err.Error(), tt.errorContains)
 				}
 			} else {
 				assert.NoError(t, err)
@@ -100,7 +91,6 @@ func TestCleanupCmdFlagParsing(t *testing.T) {
 	cmd.SetArgs([]string{
 		"cleanup",
 		"--service-id=test-service",
-		"--script=/tmp/test.sh", // Required by root command
 		"--tag-prefix=test",
 		"--consul-addr=localhost:8500",
 		"--token=test-token",
@@ -179,7 +169,6 @@ func TestCleanupCmdExecution(t *testing.T) {
 			cmd.SetArgs([]string{
 				"cleanup",
 				"--service-id=test-service",
-				"--script=/tmp/test.sh",
 				"--consul-addr=" + tt.consulAddr,
 				"--tag-prefix=test",
 			})
@@ -229,7 +218,6 @@ func TestCleanupCmdFlagRetrieval(t *testing.T) {
 	cmd.SetArgs([]string{
 		"cleanup",
 		"--service-id=test-service",
-		"--script=/tmp/test.sh",
 		"--consul-addr=localhost:9500",
 		"--tag-prefix=test-prefix",
 		"--token=test-token",
@@ -285,7 +273,6 @@ func TestCleanupCmdSuccessFlow(t *testing.T) {
 	cmd.SetArgs([]string{
 		"cleanup",
 		"--service-id=test-service",
-		"--script=/tmp/test.sh",
 		"--consul-addr=localhost:8500",
 		"--tag-prefix=test",
 		"--token=secret-token",

cmd/root.go

@@ -44,9 +44,7 @@ func init() {
 	rootCmd.PersistentFlags().StringVar(&cfgFile, "config", "", "config file (default is $HOME/.tagit.yaml)")
 	rootCmd.PersistentFlags().StringP("consul-addr", "c", "127.0.0.1:8500", "consul address")
 	rootCmd.PersistentFlags().StringP("service-id", "s", "", "consul service id")
-	rootCmd.MarkPersistentFlagRequired("service-id")
 	rootCmd.PersistentFlags().StringP("script", "x", "", "path to script used to generate tags")
-	rootCmd.MarkPersistentFlagRequired("script")
 	rootCmd.PersistentFlags().StringP("tag-prefix", "p", "tagged", "prefix to be added to tags")
 	rootCmd.PersistentFlags().StringP("interval", "i", "60s", "interval to run the script")
 	rootCmd.PersistentFlags().StringP("token", "t", "", "consul token")

cmd/run.go

@@ -81,11 +81,19 @@ example: tagit run -s my-super-service -x '/tmp/tag-role.sh'
 			logger.Error("Failed to get service-id flag", "error", err)
 			return err
 		}
+		if serviceID == "" {
+			logger.Error("Service ID is required")
+			return fmt.Errorf("service-id is required")
+		}
 		script, err := cmd.InheritedFlags().GetString("script")
 		if err != nil {
 			logger.Error("Failed to get script flag", "error", err)
 			return err
 		}
+		if script == "" {
+			logger.Error("Script is required")
+			return fmt.Errorf("script is required")
+		}
 		tagPrefix, err := cmd.InheritedFlags().GetString("tag-prefix")
 		if err != nil {
 			logger.Error("Failed to get tag-prefix flag", "error", err)

cmd/run_test.go

@@ -30,13 +30,13 @@ func TestRunCmd(t *testing.T) {
 			name:          "Missing required service-id",
 			args:          []string{"run", "--script=/tmp/test.sh"},
 			expectError:   true,
-			errorContains: "required flag(s) \"service-id\" not set",
+			errorContains: "service-id is required",
 		},
 		{
 			name:          "Missing required script",
 			args:          []string{"run", "--service-id=test-service"},
 			expectError:   true,
-			errorContains: "required flag(s) \"script\" not set",
+			errorContains: "script is required",
 		},
 		{
 			name:          "Invalid interval format",
@@ -64,9 +64,7 @@ func TestRunCmd(t *testing.T) {
 			cmd := &cobra.Command{Use: "tagit"}
 			cmd.PersistentFlags().StringP("consul-addr", "c", "127.0.0.1:8500", "consul address")
 			cmd.PersistentFlags().StringP("service-id", "s", "", "consul service id")
-			cmd.MarkPersistentFlagRequired("service-id")
 			cmd.PersistentFlags().StringP("script", "x", "", "path to script used to generate tags")
-			cmd.MarkPersistentFlagRequired("script")
 			cmd.PersistentFlags().StringP("tag-prefix", "p", "tagged", "prefix to be added to tags")
 			cmd.PersistentFlags().StringP("interval", "i", "60s", "interval to run the script")
 			cmd.PersistentFlags().StringP("token", "t", "", "consul token")
@@ -98,8 +96,7 @@ func TestRunCmd(t *testing.T) {
 				if tt.expectError {
 					assert.Error(t, err)
 					if tt.errorContains != "" {
-						output := buf.String()
-						assert.Contains(t, output, tt.errorContains)
+						assert.Contains(t, err.Error(), tt.errorContains)
 					}
 				} else {
 					assert.NoError(t, err)