bug(flags): required flags with default value still failed validation

When a required flag had a default value, execution still failed, saying
that the flag has not been provided.

This change fixes that behavior by ensuring that default values are
taken under consideration when checking whether a flag is missing or
not.

NOTE: environment variables will still override default values.

Author: arikkfir

execute_test.go

@@ -221,4 +221,35 @@ Flags:
 		With(t).Verify(rootPostRunHook.providedExitCode).Will(EqualTo(exitCode)).OrFail()
 	})
 
+	t.Run("missing required flags fail execution", func(t *testing.T) {
+		type ActionWithRequiredFlag struct {
+			TrackingAction
+			MyFlag string `required:"true"`
+		}
+		action := &ActionWithRequiredFlag{}
+		ctx := context.Background()
+		root := MustNew("cmd", "desc", "long desc", action, nil, nil)
+
+		b := &bytes.Buffer{}
+		With(t).Verify(Execute(ctx, b, root, nil, nil)).Will(EqualTo(ExitCodeMisconfiguration)).OrFail()
+		With(t).Verify(action.TrackingAction.callTime).Will(BeNil()).OrFail()
+		With(t).Verify(b.String()).Will(EqualTo("required flag is missing: --my-flag\nUsage: cmd [--help] --my-flag=VALUE\n")).OrFail()
+	})
+
+	t.Run("required flags with default value do not fail execution", func(t *testing.T) {
+		type ActionWithRequiredFlag struct {
+			TrackingAction
+			MyFlag string `required:"true"`
+		}
+		action := &ActionWithRequiredFlag{
+			MyFlag: "abc",
+		}
+		ctx := context.Background()
+		root := MustNew("cmd", "desc", "long desc", action, nil, nil)
+
+		b := &bytes.Buffer{}
+		With(t).Verify(Execute(ctx, b, root, nil, nil)).Will(EqualTo(ExitCodeSuccess)).OrFail()
+		With(t).Verify(action.TrackingAction.callTime).Will(Not(BeNil())).OrFail()
+		With(t).Verify(b.String()).Will(BeEmpty()).OrFail()
+	})
 }

flag_set.go

@@ -363,20 +363,29 @@ func (fs *flagSet) apply(envVars map[string]string, args []string) error {
 	// Iterate flags and define them in the stdlib FlagSet
 	for _, mfd := range mergedFlagDefs {
 
-		// Set the value to the flag's corresponding environment variable, if one was given
-		if v, found := envVars[*mfd.EnvVarName]; found {
-			if err := mfd.setValue(v); err != nil {
-				return err
-			}
-		}
-
 		// By definition, for the same name - all flags have the same "HasValue" value, so it should be safe to just
 		// take it from the first one
 		if mfd.HasValue {
+
+			// Set the field's default value so it's marked as "applied" (and thus the "required" validation will ignore it)
+			if mfd.DefaultValue != "" {
+				if err := mfd.setValue(mfd.DefaultValue); err != nil {
+					return fmt.Errorf("failed applying default value for flag '%s': %w", mfd.Name, err)
+				}
+			}
 			stdFs.Func(mfd.Name, "", func(v string) error { return mfd.setValue(v) })
+
 		} else {
 			stdFs.BoolFunc(mfd.Name, "", func(string) error { return mfd.setValue("true") })
 		}
+
+		// Set the value to the flag's corresponding environment variable, if one was given
+		// Important this is done here, so it overrides the default value set earlier
+		if v, found := envVars[*mfd.EnvVarName]; found {
+			if err := mfd.setValue(v); err != nil {
+				return err
+			}
+		}
 	}
 
 	// Parse the given arguments, which will result in all CLI flags being set