commit 87e3f51b073ae1e1f981abf1363e2a012c272421
author Jonathan Amsterdam <jba@google.com> Wed Sep 23 11:34:16 2020 -0400
committer Jonathan Amsterdam <jba@google.com> Wed Sep 23 21:22:47 2020 +0000
tree 6cf86ef27d27518502d06da172f20468c2eec60b
parent 6b753a8a4799e21cb30c241ab6d4b25b3d405eef

internal/config,others: GetEnvInt dies on parse error

Have config.GetEnvInt terminate the program if the environment
variable's value is non-empty but not an integer. That's clearly
a bug.

Also, use it in more places.

Change-Id: Ibcc192853e4b5fc01dad709de4de2e5db6fff709
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/256760
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Julie Qiu <julie@golang.org>

cmd/worker/main.go

diff --git a/cmd/worker/main.go b/cmd/worker/main.go
index f3873e5..d9fd04e 100644
--- a/cmd/worker/main.go
+++ b/cmd/worker/main.go
@@ -12,7 +12,6 @@
 	"flag"
 	"net/http"
 	"os"
-	"strconv"
 	"strings"
 	"time"
 
@@ -37,7 +36,7 @@
 )
 
 var (
-	timeout   = config.GetEnv("GO_DISCOVERY_WORKER_TIMEOUT_MINUTES", "10")
+	timeout   = config.GetEnvInt("GO_DISCOVERY_WORKER_TIMEOUT_MINUTES", 10)
 	queueName = config.GetEnv("GO_DISCOVERY_WORKER_TASK_QUEUE", "")
 	workers   = flag.Int("workers", 10, "number of concurrent requests to the fetch service, when running locally")
 	// flag used in call to safehtml/template.TrustedSourceFromFlag
@@ -139,10 +138,6 @@
 		go http.ListenAndServe(cfg.DebugAddr("localhost:8001"), dcensusServer)
 	}
 
-	handlerTimeout, err := strconv.Atoi(timeout)
-	if err != nil {
-		log.Fatalf(ctx, "strconv.Atoi(%q): %v", timeout, err)
-	}
 	iap := middleware.Identity()
 	if aud := os.Getenv("GO_DISCOVERY_IAP_AUDIENCE"); aud != "" {
 		iap = middleware.ValidateIAPHeader(aud)
@@ -150,7 +145,7 @@
 
 	mw := middleware.Chain(
 		middleware.RequestLog(cmdconfig.Logger(ctx, cfg, "worker-log")),
-		middleware.Timeout(time.Duration(handlerTimeout)*time.Minute),
+		middleware.Timeout(time.Duration(timeout)*time.Minute),
 		iap,
 		middleware.Experiment(experimenter),
 	)

internal/config/config.go

diff --git a/internal/config/config.go b/internal/config/config.go
index 6ea2452..8d88576 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -44,11 +44,15 @@
 // GetEnvInt looks up the given key from the environment and expects an integer,
 // returning the integer value if it exists, and otherwise returning the given
 // fallback value.
+// If the environment variable has a value but it can't be parsed as an integer,
+// GetEnvInt terminates the program.
 func GetEnvInt(key string, fallback int) int {
-	if valueStr, ok := os.LookupEnv(key); ok {
-		if value, err := strconv.Atoi(valueStr); err == nil {
-			return value
+	if s, ok := os.LookupEnv(key); ok {
+		v, err := strconv.Atoi(s)
+		if err != nil {
+			log.Fatalf("bad value %q for %s: %v", s, key, err)
 		}
+		return v
 	}
 	return fallback
 }

internal/fetch/load.go

diff --git a/internal/fetch/load.go b/internal/fetch/load.go
index 1a19892..fca6941 100644
--- a/internal/fetch/load.go
+++ b/internal/fetch/load.go
@@ -23,13 +23,13 @@
 	"path"
 	"runtime"
 	"sort"
-	"strconv"
 	"strings"
 
 	"github.com/google/safehtml"
 	"github.com/google/safehtml/template"
 	"go.opencensus.io/trace"
 	"golang.org/x/pkgsite/internal"
+	"golang.org/x/pkgsite/internal/config"
 	"golang.org/x/pkgsite/internal/derrors"
 	"golang.org/x/pkgsite/internal/fetch/dochtml"
 	"golang.org/x/pkgsite/internal/fetch/internal/doc"
@@ -347,14 +347,9 @@
 var maxModuleZipSize int64 = math.MaxInt64
 
 func init() {
-	m := os.Getenv("GO_DISCOVERY_MAX_MODULE_ZIP_MI")
-	if m != "" {
-		v, err := strconv.ParseInt(m, 10, 64)
-		if err != nil {
-			log.Errorf(context.Background(), "could not parse GO_DISCOVERY_MAX_MODULE_ZIP_MI value %q", v)
-		} else {
-			maxModuleZipSize = v * mib
-		}
+	v := config.GetEnvInt("GO_DISCOVERY_MAX_MODULE_ZIP_MI", -1)
+	if v > 0 {
+		maxModuleZipSize = int64(v) * mib
 	}
 }
 
@@ -362,17 +357,10 @@
 
 func init() {
 	ctx := context.Background()
-	m := os.Getenv("GO_DISCOVERY_MAX_IN_FLIGHT_ZIP_MI")
-	if m != "" {
-		mebis, err := strconv.ParseUint(m, 10, 64)
-		if err != nil {
-			log.Fatalf(ctx, "could not parse GO_DISCOVERY_MAX_IN_FLIGHT_ZIP_MI value %q", m)
-		} else if mebis == 0 {
-			log.Fatalf(ctx, "bad value for GO_DISCOVERY_MAX_IN_FLIGHT_ZIP_MI: %d. Must be >= 1.", mebis)
-		} else {
-			log.Infof(ctx, "shedding load over %dMi", mebis)
-			zipLoadShedder.maxSizeInFlight = mebis * mib
-		}
+	mebis := config.GetEnvInt("GO_DISCOVERY_MAX_IN_FLIGHT_ZIP_MI", -1)
+	if mebis > 0 {
+		log.Infof(ctx, "shedding load over %dMi", mebis)
+		zipLoadShedder.maxSizeInFlight = uint64(mebis) * mib
 	}
 }