Merge pull request #299 from maxmind/greg/fix-verbose-env-variable

Fix GEOIPUPDATE_VERBOSE handling. Closes #298.

CHANGELOG.md

@@ -2,11 +2,6 @@
 
 ## 7.0.0
 
-* `geoipupdate` now supports retrying on more types of errors
-  such as HTTP2 INTERNAL_ERROR.
-* Now `geoipupdate` doesn't requires the user to specify the config file
-  even if all the other arguments are set via the environment variables.
-  Reported by jsf84ksnf. GitHub #284.
 * BREAKING CHANGE: Improvements to the HTTP download API.
   The client now calls two new endpoints:
   * `/geoip/updates/metadata` which is responsible for getting information about
@@ -24,6 +19,15 @@
   internals and provide a simpler and easier to use package. Many
   previously exposed methods and types are now either internal only or have
   been removed.
+* BREAKING CHANGE: If set, `GEOIPUPDATE_VERBOSE` must either be `0` or `1`.
+  All other values will return an error.
+* Setting `GEOIPUPDATE_VERBOSE` to `1` now works as expected. In the 6.0.0 and
+  6.1.0 releases, the flag was ignored. Reported by pmcevoy. GitHub #298.
+* `geoipupdate` now supports retrying on more types of errors
+  such as HTTP2 INTERNAL_ERROR.
+* Now `geoipupdate` doesn't requires the user to specify the config file
+  even if all the other arguments are set via the environment variables.
+  Reported by jsf84ksnf. GitHub #284.
 
 ## 6.1.0 (2024-01-09)
 

cmd/geoipupdate/args.go

@@ -21,7 +21,7 @@ type Args struct {
 func getArgs() *Args {
 	confFileDefault := vars.DefaultConfigFile
 	// Set the default config file only if it exists.
-	// Othwerwise, geoipupdate requires the user to specify the config file
+	// Otherwise, geoipupdate requires the user to specify the config file
 	// even if all the other arguments are set via the environment variables.
 	if _, err := os.Stat(confFileDefault); errors.Is(err, os.ErrNotExist) {
 		confFileDefault = ""

cmd/geoipupdate/main.go

@@ -32,13 +32,21 @@ func main() {
 
 	args := getArgs()
 
-	config, err := geoipupdate.NewConfig(
+	opts := []geoipupdate.Option{
 		geoipupdate.WithConfigFile(args.ConfigFile),
 		geoipupdate.WithDatabaseDirectory(args.DatabaseDirectory),
 		geoipupdate.WithParallelism(args.Parallelism),
-		geoipupdate.WithVerbose(args.Verbose),
-		geoipupdate.WithOutput(args.Output),
-	)
+	}
+
+	if args.Output {
+		opts = append(opts, geoipupdate.WithOutput)
+	}
+
+	if args.Verbose {
+		opts = append(opts, geoipupdate.WithVerbose)
+	}
+
+	config, err := geoipupdate.NewConfig(opts...)
 	if err != nil {
 		log.Fatalf("Error loading configuration: %s", err)
 	}

doc/docker.md

@@ -40,7 +40,7 @@ The following are optional:
   of files downloaded from the server. This option is either `0` or `1`. The
   default is `0`.
 * `GEOIPUPDATE_VERBOSE` - Enable verbose mode. Prints out the steps that
-  `geoipupdate` takes. Set to **anything** (e.g., `1`) to enable.
+  `geoipupdate` takes. Set to `1` to enable.
 * `GEOIPUPDATE_CONF_FILE` - The path of a configuration file to be used by
   `geoipupdate`.
 * `GEOIPUPDATE_DB_DIR` - The directory where geoipupdate will download the

internal/geoipupdate/config.go

@@ -87,22 +87,16 @@ func WithDatabaseDirectory(dir string) Option {
 	}
 }
 
-// WithVerbose returns an Option that sets the Verbose
-// value of a config.
-func WithVerbose(val bool) Option {
-	return func(c *Config) error {
-		c.Verbose = val
-		return nil
-	}
+// WithVerbose enable verbose output for the config.
+func WithVerbose(c *Config) error {
+	c.Verbose = true
+	return nil
 }
 
-// WithOutput returns an Option that sets the Output
-// value of a config.
-func WithOutput(val bool) Option {
-	return func(c *Config) error {
-		c.Output = val
-		return nil
-	}
+// WithOutput enables JSON output for the config.
+func WithOutput(c *Config) error {
+	c.Output = true
+	return nil
 }
 
 // WithConfigFile returns an Option that sets the configuration
@@ -348,7 +342,7 @@ func setConfigFromEnv(config *Config) error {
 
 	if value, ok := os.LookupEnv("GEOIPUPDATE_PRESERVE_FILE_TIMES"); ok {
 		if value != "0" && value != "1" {
-			return errors.New("`PreserveFileTimes' must be 0 or 1")
+			return errors.New("`GEOIPUPDATE_PRESERVE_FILE_TIMES' must be 0 or 1")
 		}
 		config.PreserveFileTimes = value == "1"
 	}
@@ -370,7 +364,10 @@ func setConfigFromEnv(config *Config) error {
 	}
 
 	if value, ok := os.LookupEnv("GEOIPUPDATE_VERBOSE"); ok {
-		config.Verbose = value != ""
+		if value != "0" && value != "1" {
+			return errors.New("`GEOIPUPDATE_VERBOSE' must be 0 or 1")
+		}
+		config.Verbose = value == "1"
 	}
 
 	return nil

internal/geoipupdate/config_test.go

@@ -702,7 +702,7 @@ func TestSetConfigFromEnv(t *testing.T) {
 			Env: map[string]string{
 				"GEOIPUPDATE_PRESERVE_FILE_TIMES": "1a",
 			},
-			Err: "`PreserveFileTimes' must be 0 or 1",
+			Err: "`GEOIPUPDATE_PRESERVE_FILE_TIMES' must be 0 or 1",
 		},
 		{
 			Description: "RetryFor needs a unit",
@@ -732,6 +732,13 @@ func TestSetConfigFromEnv(t *testing.T) {
 			},
 			Err: "parallelism should be greater than 0, got '0'",
 		},
+		{
+			Description: "Invalid Verbose",
+			Env: map[string]string{
+				"GEOIPUPDATE_VERBOSE": "1a",
+			},
+			Err: "`GEOIPUPDATE_VERBOSE' must be 0 or 1",
+		},
 	}
 
 	for _, test := range tests {
@@ -773,9 +780,9 @@ func TestSetConfigFromFlags(t *testing.T) {
 			Description: "All option flag related config set",
 			Flags: []Option{
 				WithDatabaseDirectory("/tmp/db"),
-				WithOutput(true),
+				WithOutput,
 				WithParallelism(2),
-				WithVerbose(true),
+				WithVerbose,
 			},
 			Expected: Config{
 				DatabaseDirectory: filepath.Clean("/tmp/db"),