WIP:better

kubernetes · Jun 13, 2024 · a5b4076 · a5b4076
1 parent 72e9f6f
commit a5b4076
Show file tree

Hide file tree

Showing 3 changed files with 61 additions and 31 deletions.
diff --git a/env.go b/env.go
@@ -25,6 +25,8 @@ import (
 	"strconv"
 	"strings"
 	"time"
+
+	"github.com/spf13/pflag"
 )
 
 func envString(def string, key string, alts ...string) string {
@@ -33,15 +35,21 @@ func envString(def string, key string, alts ...string) string {
 	}
 	for _, alt := range alts {
 		if val := os.Getenv(alt); val != "" {
-			fmt.Fprintf(os.Stderr, "env %s has been deprecated, use %s instead\n", alt, key)
+			fmt.Fprintf(os.Stderr, "env $%s has been deprecated, use $%s instead\n", alt, key)
 			return val
 		}
 	}
 	return def
 }
-func envFlagString(key string, def string, usage string, alts ...string) string {
+func envFlagString(key string, def string, usage string, alts ...string) *string {
 	registerEnvFlag(key, "string", usage)
-	return envString(def, key, alts...)
+	val := envString(def, key, alts...)
+	// also expose it as a flag, for easier testing
+	flName := "__env__" + key
+	flHelp := "DO NOT SET THIS FLAG EXCEPT IN TESTS; use $" + key
+	newExplicitFlag(&val, flName, flHelp, pflag.String)
+	pflag.CommandLine.MarkHidden(flName)
+	return &val
 }
 
 func envStringArray(def string, key string, alts ...string) []string {
@@ -54,7 +62,7 @@ func envStringArray(def string, key string, alts ...string) []string {
 	}
 	for _, alt := range alts {
 		if val := os.Getenv(alt); val != "" {
-			fmt.Fprintf(os.Stderr, "env %s has been deprecated, use %s instead\n", alt, key)
+			fmt.Fprintf(os.Stderr, "env $%s has been deprecated, use $%s instead\n", alt, key)
 			return parse(val)
 		}
 	}
@@ -75,7 +83,7 @@ func envBoolOrError(def bool, key string, alts ...string) (bool, error) {
 	}
 	for _, alt := range alts {
 		if val := os.Getenv(alt); val != "" {
-			fmt.Fprintf(os.Stderr, "env %s has been deprecated, use %s instead\n", alt, key)
+			fmt.Fprintf(os.Stderr, "env $%s has been deprecated, use $%s instead\n", alt, key)
 			return parse(alt, val)
 		}
 	}
@@ -105,7 +113,7 @@ func envIntOrError(def int, key string, alts ...string) (int, error) {
 	}
 	for _, alt := range alts {
 		if val := os.Getenv(alt); val != "" {
-			fmt.Fprintf(os.Stderr, "env %s has been deprecated, use %s instead\n", alt, key)
+			fmt.Fprintf(os.Stderr, "env $%s has been deprecated, use $%s instead\n", alt, key)
 			return parse(alt, val)
 		}
 	}
@@ -135,7 +143,7 @@ func envFloatOrError(def float64, key string, alts ...string) (float64, error) {
 	}
 	for _, alt := range alts {
 		if val := os.Getenv(alt); val != "" {
-			fmt.Fprintf(os.Stderr, "env %s has been deprecated, use %s instead\n", alt, key)
+			fmt.Fprintf(os.Stderr, "env $%s has been deprecated, use $%s instead\n", alt, key)
 			return parse(alt, val)
 		}
 	}
@@ -165,7 +173,7 @@ func envDurationOrError(def time.Duration, key string, alts ...string) (time.Dur
 	}
 	for _, alt := range alts {
 		if val := os.Getenv(alt); val != "" {
-			fmt.Fprintf(os.Stderr, "env %s has been deprecated, use %s instead\n", alt, key)
+			fmt.Fprintf(os.Stderr, "env $%s has been deprecated, use $%s instead\n", alt, key)
 			return parse(alt, val)
 		}
 	}
@@ -181,6 +189,38 @@ func envDuration(def time.Duration, key string, alts ...string) time.Duration {
 	return val
 }
 
+// explicitFlag is a pflag.Value which only sets the real value if the flag is
+// set to a non-zero-value.
+type explicitFlag[T comparable] struct {
+	pflag.Value
+	realPtr *T
+	flagPtr *T
+}
+
+// newExplicitFlag allocates an explicitFlag
+func newExplicitFlag[T comparable](ptr *T, name, usage string, fn func(name string, value T, usage string) *T) {
+	h := &explicitFlag[T]{
+		realPtr: ptr,
+	}
+	var zero T
+	h.flagPtr = fn(name, zero, usage)
+	fl := pflag.CommandLine.Lookup(name)
+	// wrap the original pflag.Value with our own
+	h.Value = fl.Value
+	fl.Value = h
+}
+
+func (h *explicitFlag[T]) Set(val string) error {
+	if err := h.Value.Set(val); err != nil {
+		return err
+	}
+	var zero T
+	if v := *h.flagPtr; v != zero {
+		*h.realPtr = v
+	}
+	return nil
+}
+
 // envFlag is like a flag in that it is declared with a type, validated, and
 // shows up in help messages, but can only be set by env-var, not on the CLI.
 // This is useful for things like passwords, which should not be on the CLI

diff --git a/main.go b/main.go
@@ -512,13 +512,13 @@ func main() {
 
 	if *flDeprecatedPassword != "" {
 		log.V(0).Info("setting $GITSYNC_PASSWORD from deprecated --password")
-		flPassword = *flDeprecatedPassword
+		*flPassword = *flDeprecatedPassword
 	}
 	if *flUsername != "" {
-		if flPassword == "" && *flPasswordFile == "" {
+		if *flPassword == "" && *flPasswordFile == "" {
 			fatalConfigError(log, true, "required flag: $GITSYNC_PASSWORD or --password-file must be specified when --username is specified")
 		}
-		if flPassword != "" && *flPasswordFile != "" {
+		if *flPassword != "" && *flPasswordFile != "" {
 			fatalConfigError(log, true, "invalid flag: only one of $GITSYNC_PASSWORD and --password-file may be specified")
 		}
 		if u, err := url.Parse(*flRepo); err == nil { // it may not even parse as a URL, that's OK
@@ -527,7 +527,7 @@ func main() {
 			}
 		}
 	} else {
-		if flPassword != "" {
+		if *flPassword != "" {
 			fatalConfigError(log, true, "invalid flag: $GITSYNC_PASSWORD may only be specified when --username is specified")
 		}
 		if *flPasswordFile != "" {
@@ -620,7 +620,7 @@ func main() {
 					*flUsername = user
 				}
 				if pass, found := u.User.Password(); found {
-					flPassword = pass
+					*flPassword = pass
 				}
 				u.User = nil
 				*flRepo = u.String()
@@ -631,7 +631,7 @@ func main() {
 		cred := credential{
 			URL:          *flRepo,
 			Username:     *flUsername,
-			Password:     flPassword,
+			Password:     *flPassword,
 			PasswordFile: *flPasswordFile,
 		}
 		*flCredentials = append([]credential{cred}, (*flCredentials)...)

diff --git a/test_e2e.sh b/test_e2e.sh
@@ -268,19 +268,12 @@ HTTP_PORT=9376
 METRIC_GOOD_SYNC_COUNT='git_sync_count_total{status="success"}'
 METRIC_FETCH_COUNT='git_fetch_count_total'
 
-# Call this like running a command, with one exception - the "ENVS[]" variable
-# can be set to a series of "key=value" strings.
 function GIT_SYNC() {
     #./bin/linux_amd64/git-sync "$@"
     RM="--rm"
     if [[ "${CLEANUP:-}" == 0 ]]; then
         RM=""
     fi
-    # This is a little hacky, but I'll live with it until I can't.
-    local env_args=()
-    for e in "${ENVS[@]}"; do
-        env_args+=("--env" "${e}")
-    done
     docker run \
         -i \
         ${RM} \
@@ -292,11 +285,11 @@ function GIT_SYNC() {
         -v "$REPO2":"$REPO2":ro \
         -v "$WORK":"$WORK":ro \
         -v "$(pwd)/$TEST_TOOLS":"/$TEST_TOOLS":ro \
+        --env "$EXECHOOK_ENVKEY=$EXECHOOK_ENVVAL" \
         -v "$RUNLOG":/var/log/runs \
         -v "$DOT_SSH/1/id_test":"/ssh/secret.1":ro \
         -v "$DOT_SSH/2/id_test":"/ssh/secret.2":ro \
         -v "$DOT_SSH/3/id_test":"/ssh/secret.3":ro \
-        "${env_args[@]}" \
         "${IMAGE}" \
             -v=6 \
             --add-user \
@@ -1784,35 +1777,35 @@ function e2e::auth_http_password() {
     IP=$(docker_ip "$CTR")
 
     # Try with wrong username
-    ENVS=(GITSYNC_PASSWORD=testpass FOO=BAR)
     assert_fail \
         GIT_SYNC \
             --one-time \
             --repo="http://$IP/repo" \
             --root="$ROOT" \
             --link="link" \
-            --username="wrong"
+            --username="wrong" \
+            --__env__GITSYNC_PASSWORD="testpass"
     assert_file_absent "$ROOT/link/file"
 
     # Try with wrong password
-    ENVS=(GITSYNC_PASSWORD=wrong)
     assert_fail \
         GIT_SYNC \
             --one-time \
             --repo="http://$IP/repo" \
             --root="$ROOT" \
             --link="link" \
-            --username="testuser"
+            --username="testuser" \
+            --__env__GITSYNC_PASSWORD="wrong"
     assert_file_absent "$ROOT/link/file"
 
     # Try with the right password
-    ENVS=(GITSYNC_PASSWORD=testpass)
     GIT_SYNC \
         --one-time \
         --repo="http://$IP/repo" \
         --root="$ROOT" \
         --link="link" \
-        --username="testuser"
+        --username="testuser" \
+        --__env__GITSYNC_PASSWORD="testpass" \
 
     assert_link_exists "$ROOT/link"
     assert_file_exists "$ROOT/link/file"
@@ -2200,7 +2193,6 @@ function e2e::exechook_success() {
     echo "$FUNCNAME 1" > "$REPO/file"
     git -C "$REPO" commit -qam "$FUNCNAME 1"
 
-    ENVS=("$EXECHOOK_ENVKEY=$EXECHOOK_ENVVAL")
     GIT_SYNC \
         --period=100ms \
         --repo="file://$REPO" \
@@ -2255,7 +2247,6 @@ function e2e::exechook_fail_retry() {
 # Test exechook-success with --one-time
 ##############################################
 function e2e::exechook_success_once() {
-    ENVS=("$EXECHOOK_ENVKEY=$EXECHOOK_ENVVAL")
     GIT_SYNC \
         --one-time \
         --repo="file://$REPO" \
@@ -2310,7 +2301,6 @@ function e2e::exechook_startup_after_crash() {
     # No changes to repo
 
     cat /dev/null > "$RUNLOG"
-    ENVS=("$EXECHOOK_ENVKEY=$EXECHOOK_ENVVAL")
     GIT_SYNC \
         --one-time \
         --repo="file://$REPO" \