Extract typed environment variable helpers

These helpers should make parsing typed environment variables more
consistent.

8 files changed, 180 insertions, 11 deletions

diff --git a/cmd/gitaly-hooks/hooks.go b/cmd/gitaly-hooks/hooks.go
index 897ce30a8..03b33ddb8 100644
--- a/cmd/gitaly-hooks/hooks.go
+++ b/cmd/gitaly-hooks/hooks.go
@@ -7,7 +7,6 @@ import (
 	"io"
 	"log"
 	"os"
-	"strconv"
 	"strings"
 
 	"github.com/sirupsen/logrus"
@@ -18,6 +17,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config/prometheus"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/hook"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitlab"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/helper/env"
 	gitalylog "gitlab.com/gitlab-org/gitaly/v14/internal/log"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/stream"
@@ -163,7 +163,7 @@ func dialGitaly(payload git.HooksPayload) (*grpc.ClientConn, error) {
 func gitPushOptions() []string {
 	var gitPushOptions []string
 
-	gitPushOptionCount, err := strconv.Atoi(os.Getenv("GIT_PUSH_OPTION_COUNT"))
+	gitPushOptionCount, err := env.GetInt("GIT_PUSH_OPTION_COUNT", 0)
 	if err != nil {
 		return gitPushOptions
 	}
diff --git a/cmd/gitaly-wrapper/main.go b/cmd/gitaly-wrapper/main.go
index 1239d4503..3ac422070 100644
--- a/cmd/gitaly-wrapper/main.go
+++ b/cmd/gitaly-wrapper/main.go
@@ -13,6 +13,7 @@ import (
 
 	"github.com/sirupsen/logrus"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/bootstrap"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/helper/env"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/log"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/ps"
 	"golang.org/x/sys/unix"
@@ -177,5 +178,6 @@ func pidFile() string {
 }
 
 func jsonLogging() bool {
-	return os.Getenv(envJSONLogging) == "true"
+	enabled, _ := env.GetBool(envJSONLogging, false)
+	return enabled
 }
diff --git a/internal/bootstrap/bootstrap.go b/internal/bootstrap/bootstrap.go
index 5f1f87e04..ef414184d 100644
--- a/internal/bootstrap/bootstrap.go
+++ b/internal/bootstrap/bootstrap.go
@@ -10,6 +10,7 @@ import (
 
 	"github.com/cloudflare/tableflip"
 	log "github.com/sirupsen/logrus"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/helper/env"
 	"golang.org/x/sys/unix"
 )
 
@@ -65,7 +66,7 @@ type upgrader interface {
 // gitaly-wrapper is supposed to set EnvUpgradesEnabled in order to enable graceful upgrades
 func New() (*Bootstrap, error) {
 	pidFile := os.Getenv(EnvPidFile)
-	_, upgradesEnabled := os.LookupEnv(EnvUpgradesEnabled)
+	upgradesEnabled, _ := env.GetBool(EnvUpgradesEnabled, false)
 
 	// PIDFile is optional, if provided tableflip will keep it updated
 	upg, err := tableflip.New(tableflip.Options{
diff --git a/internal/gitaly/config/config.go b/internal/gitaly/config/config.go
index d1075c941..77802a546 100644
--- a/internal/gitaly/config/config.go
+++ b/internal/gitaly/config/config.go
@@ -21,6 +21,7 @@ import (
 	internallog "gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config/log"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config/prometheus"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config/sentry"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/helper/env"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper/text"
 	"golang.org/x/sys/unix"
 )
@@ -354,7 +355,8 @@ func (cfg *Cfg) validateStorages() error {
 }
 
 func SkipHooks() bool {
-	return os.Getenv("GITALY_TESTING_NO_GIT_HOOKS") == "1"
+	enabled, _ := env.GetBool("GITALY_TESTING_NO_GIT_HOOKS", false)
+	return enabled
 }
 
 // SetGitPath populates the variable GitPath with the path to the `git`
diff --git a/internal/gitaly/rubyserver/rubyserver.go b/internal/gitaly/rubyserver/rubyserver.go
index 86738c4df..6292d1d66 100644
--- a/internal/gitaly/rubyserver/rubyserver.go
+++ b/internal/gitaly/rubyserver/rubyserver.go
@@ -17,6 +17,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/rubyserver/balancer"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/helper/env"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/supervisor"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/version"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
@@ -32,9 +33,9 @@ var (
 )
 
 func init() {
-	timeout64, err := strconv.ParseInt(os.Getenv("GITALY_RUBY_CONNECT_TIMEOUT"), 10, 32)
-	if err == nil && timeout64 > 0 {
-		ConnectTimeout = time.Duration(timeout64) * time.Second
+	timeout, err := env.GetInt("GITALY_RUBY_CONNECT_TIMEOUT", 0)
+	if err == nil && timeout > 0 {
+		ConnectTimeout = time.Duration(timeout) * time.Second
 	}
 }
 
diff --git a/internal/helper/env/env.go b/internal/helper/env/env.go
new file mode 100644
index 000000000..24bc36510
--- /dev/null
+++ b/internal/helper/env/env.go
@@ -0,0 +1,39 @@
+package env
+
+import (
+	"fmt"
+	"os"
+	"strconv"
+)
+
+// GetBool fetches and parses a boolean typed environment variable
+//
+// If the variable is empty, returns `fallback` and no error.
+// If there is an error, returns `fallback` and the error.
+func GetBool(name string, fallback bool) (bool, error) {
+	s := os.Getenv(name)
+	if s == "" {
+		return fallback, nil
+	}
+	v, err := strconv.ParseBool(s)
+	if err != nil {
+		return fallback, fmt.Errorf("get bool %s: %w", name, err)
+	}
+	return v, nil
+}
+
+// GetInt fetches and parses an integer typed environment variable
+//
+// If the variable is empty, returns `fallback` and no error.
+// If there is an error, returns `fallback` and the error.
+func GetInt(name string, fallback int) (int, error) {
+	s := os.Getenv(name)
+	if s == "" {
+		return fallback, nil
+	}
+	v, err := strconv.Atoi(s)
+	if err != nil {
+		return fallback, fmt.Errorf("get int %s: %w", name, err)
+	}
+	return v, nil
+}
diff --git a/internal/helper/env/env_test.go b/internal/helper/env/env_test.go
new file mode 100644
index 000000000..148144600
--- /dev/null
+++ b/internal/helper/env/env_test.go
@@ -0,0 +1,124 @@
+package env_test
+
+import (
+	"errors"
+	"fmt"
+	"strconv"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/helper/env"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
+)
+
+func TestGetBool(t *testing.T) {
+	for _, tc := range []struct {
+		value         string
+		fallback      bool
+		expected      bool
+		expectedErrIs error
+	}{
+		{
+			value:    "true",
+			expected: true,
+		},
+		{
+			value:    "false",
+			expected: false,
+		},
+		{
+			value:    "1",
+			expected: true,
+		},
+		{
+			value:    "0",
+			expected: false,
+		},
+		{
+			value:    "",
+			expected: false,
+		},
+		{
+			value:    "",
+			fallback: true,
+			expected: true,
+		},
+		{
+			value:         "bad",
+			expected:      false,
+			expectedErrIs: strconv.ErrSyntax,
+		},
+		{
+			value:         "bad",
+			fallback:      true,
+			expected:      true,
+			expectedErrIs: strconv.ErrSyntax,
+		},
+	} {
+		t.Run(fmt.Sprintf("value=%s,fallback=%t", tc.value, tc.fallback), func(t *testing.T) {
+			cleanup := testhelper.ModifyEnvironment(t, "TEST_BOOL", tc.value)
+			t.Cleanup(cleanup)
+
+			result, err := env.GetBool("TEST_BOOL", tc.fallback)
+
+			if tc.expectedErrIs != nil {
+				assert.Error(t, err)
+				assert.True(t, errors.Is(err, tc.expectedErrIs), err)
+			} else {
+				assert.NoError(t, err)
+			}
+
+			assert.Equal(t, tc.expected, result)
+		})
+	}
+}
+
+func TestGetInt(t *testing.T) {
+	for _, tc := range []struct {
+		value         string
+		fallback      int
+		expected      int
+		expectedErrIs error
+	}{
+		{
+			value:    "3",
+			expected: 3,
+		},
+		{
+			value:    "",
+			expected: 0,
+		},
+		{
+			value:    "",
+			fallback: 3,
+			expected: 3,
+		},
+		{
+			value:         "bad",
+			expected:      0,
+			expectedErrIs: strconv.ErrSyntax,
+		},
+		{
+			value:         "bad",
+			fallback:      3,
+			expected:      3,
+			expectedErrIs: strconv.ErrSyntax,
+		},
+	} {
+		t.Run(fmt.Sprintf("value=%s,fallback=%d", tc.value, tc.fallback), func(t *testing.T) {
+			cleanup := testhelper.ModifyEnvironment(t, "TEST_INT", tc.value)
+			t.Cleanup(cleanup)
+
+			result, err := env.GetInt("TEST_INT", tc.fallback)
+
+			if tc.expectedErrIs != nil {
+				assert.Error(t, err)
+				assert.True(t, errors.Is(err, tc.expectedErrIs), err)
+			} else {
+				assert.NoError(t, err)
+			}
+
+			assert.Equal(t, tc.expected, result)
+		})
+	}
+}
diff --git a/internal/metadata/featureflag/grpc_header.go b/internal/metadata/featureflag/grpc_header.go
index b735d8aba..f8b7709df 100644
--- a/internal/metadata/featureflag/grpc_header.go
+++ b/internal/metadata/featureflag/grpc_header.go
@@ -3,12 +3,12 @@ package featureflag
 import (
 	"context"
 	"fmt"
-	"os"
 	"strconv"
 	"strings"
 
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/client_golang/prometheus/promauto"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/helper/env"
 	"google.golang.org/grpc/metadata"
 )
 
@@ -18,7 +18,7 @@ var (
 	// GITALY_TESTING_ENABLE_ALL_FEATURE_FLAGS is set to "true", then all
 	// feature flags will be enabled. This is only used for testing
 	// purposes such that we can run integration tests with feature flags.
-	featureFlagsOverride = os.Getenv("GITALY_TESTING_ENABLE_ALL_FEATURE_FLAGS")
+	featureFlagsOverride, _ = env.GetBool("GITALY_TESTING_ENABLE_ALL_FEATURE_FLAGS", false)
 
 	flagChecks = promauto.NewCounterVec(
 		prometheus.CounterOpts{
@@ -35,7 +35,7 @@ const Delim = ":"
 // IsEnabled checks if the feature flag is enabled for the passed context.
 // Only returns true if the metadata for the feature flag is set to "true"
 func IsEnabled(ctx context.Context, flag FeatureFlag) bool {
-	if featureFlagsOverride == "true" {
+	if featureFlagsOverride {
 		return true
 	}