Repository navigation
Clean up GEOIPUPDATE_PROXY_USER_PASSWORD_FILE support - #571
Conversation
Before #570, geoipupdate parsed GEOIPUPDATE_ACCOUNT_ID before it read GEOIPUPDATE_ACCOUNT_ID_FILE, so an invalid GEOIPUPDATE_ACCOUNT_ID caused an error even when the file was set. Now the file value replaces the variable before parsing, so the invalid value is ignored. Add a test for this behavior. Co-Authored-By: Claude Opus 5.5 <[email protected]>
The callers of getValueFromEnvFile checked err only inside the found branch. They saw a file read error only because the helper also returned found=true with it. If the helper returned found=false with an error, the callers would ignore the error. Check err first, as for other Go functions that return an error. Co-Authored-By: Claude Opus 5.5 <[email protected]>
If both a variable and its _FILE form are set, geoipupdate uses the value from the file. The docs did not say this. Co-Authored-By: Claude Opus 5.5 <[email protected]>
No test covered the error that occurs when a _FILE variable names a file that geoipupdate cannot read. Add one test for each of the 3 _FILE variables, as each caller handles the error. The text of the OS error differs between platforms, so the tests get it from a read of the same missing file. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Each test that set a _FILE variable also set its base variable, such as GEOIPUPDATE_LICENSE_KEY. Add a test that sets only the 3 _FILE variables. Co-Authored-By: Claude Opus 5.5 <[email protected]>
These 2 tests set GEOIPUPDATE_PROXY_USER_PASSWORD_FILE, but their names said PROXY_USER_PASSWORD. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Co-Authored-By: Claude Opus 5.5 <[email protected]>
The GeoIP.conf(5) man page is built from this file, so it now also lists the variable. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Co-Authored-By: Claude Opus 5.5 <[email protected]>
This file puts a caller before the functions it calls. The function does not change. Co-Authored-By: Claude Opus 5.5 <[email protected]>
The ":=" on the next line declares err. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Read the _FILE variable first and return early. Otherwise, return the result of os.LookupEnv. This removes the res and found variables and the assignments that overwrote them. On a read error, return found=false, as is usual with a non-nil error. The callers check err before found, so this does not change behavior. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 44.44% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 2 files. (2 skipped: 2… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly identifies the primary change: cleanup of GEOIPUPDATE_PROXY_USER_PASSWORD_FILE support, including implementation, tests, and documentation. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 44.44% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 2 files. (2 skipped: 2 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 2
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
🛠️ Fix failing CI checks: 💡
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit checks each file with care
Then trims the spaces hiding there
The proxy pair comes through
Account and license too
Blank pages raise an error
And hops are neat and clear
Comment @coderabbitai help to get the list of available commands.
There was a problem hiding this comment.
🟢 Approval recommended
The localized refactor preserves behavior, with supporting tests and documentation and no unresolved findings.
No open issues 🎉
What changed in this PR
Cleans up file-backed environment configuration, expands regression coverage, and documents proxy credential file support.
Changes:
- Simplifies and renames the helper to
lookupEnvOrFile, checking errors before using results. - Adds tests for file precedence, missing files, and file-only configuration.
- Documents proxy credential files and precedence rules.
| File | Description |
|---|---|
| internal/geoipupdate/config.go | Refactors environment/file lookup and error handling. |
| internal/geoipupdate/config_test.go | Adds regression cases and corrects test names. |
| doc/GeoIP.conf.md | Documents proxy credential files and precedence. |
| doc/docker.md | Adds the proxy credential file option and precedence rules. |
| CHANGELOG.md | Records proxy credential file support. |
Review effort: Balanced
The error said "failed to open" for every os.ReadFile error. The read can also fail after the open, for example when the path is a directory. Then the error was "failed to open ...: read ...: is a directory". Co-Authored-By: Claude Opus 5.5 <[email protected]>
The old name could mean that the function reads a .env file. The new name follows os.LookupEnv, which also returns the value and whether it was found. Co-Authored-By: Claude Opus 5.5 <[email protected]>
The old comment did not say that the file takes precedence, that the file contents are trimmed, or what the returned bool means. Co-Authored-By: Claude Opus 5.5 <[email protected]>
If GEOIPUPDATE_PROXY_USER_PASSWORD_FILE pointed to an empty file, the empty value replaced ProxyUserPassword from GeoIP.conf with no error. The proxy then got no credentials, and requests failed later with a less clear error. An empty secret file is almost always a mistake, for example a secret that was mounted but not filled in. lookupEnvOrFile now returns an error if the file is empty after trimming. This also applies to the account ID and license key files. An empty file there already failed, but with "invalid account ID format" or "the `LicenseKey` option is required". The new error names the variable and the file. A plain GEOIPUPDATE_PROXY_USER_PASSWORD="" still clears the value from GeoIP.conf. Co-Authored-By: Claude Opus 5.5 <[email protected]>
withEnvVars restored the environment only if f returned normally. A failed require call in f stops the test before the restore, so the variables stayed set and caused later subtests to fail. In TestSetConfigFromEnv, one failed require call made 14 subtests fail. Restore the environment in t.Cleanup, which runs even when the test stops early. Co-Authored-By: Claude Opus 5.5 <[email protected]>
6cb5297 to
e44e59c
Compare
| } | ||
| }) | ||
|
|
||
| for key, val := range newEnvVars { |
There was a problem hiding this comment.
withEnvVars sets newEnvVars on top of the current environment. It does not unset the GEOIPUPDATE_* variables that the shell already has, so the tests depend on the developer's shell. The new "Missing ...", "Blank ..." and "Only the _FILE variables" cases add to this.
To reproduce:
GEOIPUPDATE_EDITION_IDS=GeoLite2-City GEOIPUPDATE_PROXY_USER_PASSWORD=a:b \
go test ./internal/geoipupdate -run TestSetConfigFromEnv13 subtests fail, among them "Only the _FILE variables", "Missing LICENSE_KEY_FILE" and "Blank PROXY_USER_PASSWORD_FILE". Config gets EditionIDs and proxyUserInfo from the shell.
Possible fix: unset every GEOIPUPDATE_* variable before this loop.
Claude Code (an AI agent) wrote this comment for @oschwald.
| parts := strings.SplitN(pair, "=", 2) | ||
| //nolint:usetesting // t.Setenv causes errors on Windows for some reason. | ||
| err := os.Setenv(parts[0], parts[1]) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
If one os.Setenv call fails here, require.NoError calls t.FailNow and the loop stops. os.Clearenv already ran, so the remaining original variables (PATH, HOME, TMP and others) do not come back. All later tests in the package then run with an almost empty environment. Commit e44e59c set out to prevent this kind of cascade.
assert.NoError records the failure and lets the loop finish:
| require.NoError(t, err) | |
| assert.NoError(t, err) |
Claude Code (an AI agent) wrote this comment for @oschwald.
|
|
||
| // Reset the original environment variables | ||
| for _, pair := range origEnv { | ||
| parts := strings.SplitN(pair, "=", 2) |
There was a problem hiding this comment.
On Windows, os.Environ can return names that start with =, for example =C:=C:\src. cmd.exe sets one of these for each drive. For such a pair, SplitN returns an empty key, and os.Setenv("", ...) returns an error. The restore then stops (see the comment on line 1138).
syscall.Clearenv on Windows handles this case. It searches for = from index 1. The same search here would fix it.
This code is from before this PR, but this PR moves it into the new Cleanup. I did not run this on Windows.
Claude Code (an AI agent) wrote this comment for @oschwald.
There was a problem hiding this comment.
Fixed in 403f561, which removed the restore loop (see the reply on line 1138). The helper now changes only names that start with GEOIPUPDATE_, so it does not touch a name such as =C:. I did not run this on Windows. The windows-latest CI job tests it.
Claude Code (an AI agent) wrote this reply for @horgh.
| if err != nil { | ||
| return "", false, fmt.Errorf("failed to read %s_FILE: %w", key, err) | ||
| } | ||
| value := strings.TrimSpace(string(content)) |
There was a problem hiding this comment.
strings.TrimSpace also removes leading and trailing white space from the proxy user name and password. GEOIPUPDATE_PROXY_USER_PASSWORD keeps it. For example, a file that contains user:pass (the password ends in a space) gives the password pass. The same value in the variable gives pass .
The docs do not say that geoipupdate trims the file contents. Possible fixes:
- Say in
doc/GeoIP.conf.mdanddoc/docker.mdthatgeoipupdateremoves leading and trailing white space from the file. - Remove only the trailing newline for this key.
Claude Code (an AI agent) wrote this comment for @oschwald.
There was a problem hiding this comment.
| } | ||
| value := strings.TrimSpace(string(content)) | ||
| if value == "" { | ||
| return "", false, fmt.Errorf("%s_FILE points to an empty file: %s", key, path) |
There was a problem hiding this comment.
3 small points about this error:
- It also occurs for a file that contains only white space, for example a single newline.
ls -lthen shows a non-zero size, so "empty file" can confuse the user. - The path is not quoted. If the path ends in a space, the message still looks correct.
%qshows the space. - The message gives the cause but not the fix.
For example:
return "", false, fmt.Errorf("%s_FILE is set to %q, but that file is empty or contains only white space", key, path)The "Blank ..." test cases would need the same change.
Claude Code (an AI agent) wrote this comment for @oschwald.
| - The Docker images now carry `org.opencontainers.image.*` labels. | ||
| - A prerelease no longer moves the `latest`, `v8` and `v8.1` Docker tags, or the | ||
| release marked "Latest" on GitHub. | ||
| - Added the `GEOIPUPDATE_PROXY_USER_PASSWORD_FILE` environment variable. It sets |
There was a problem hiding this comment.
Compared with 8.0.0, this release also changes GEOIPUPDATE_ACCOUNT_ID_FILE and GEOIPUPDATE_LICENSE_KEY_FILE in 3 ways that users can see:
- An empty or white-space-only file gives a new error. In 8.0.0, it gave "invalid account ID format" or "the
LicenseKeyoption is required". - A read error says "failed to read". In 8.0.0, it said "failed to open".
- An invalid
GEOIPUPDATE_ACCOUNT_IDno longer causes an error whenGEOIPUPDATE_ACCOUNT_ID_FILEis set.
Should the changelog mention these? Users who match on the error text, or who relied on the old error, get no notice.
Claude Code (an AI agent) wrote this comment for @oschwald.
There was a problem hiding this comment.
We decided not to add these. They are too minor for the changelog.
Claude Code (an AI agent) wrote this reply for @horgh.
| @@ -1040,6 +1124,21 @@ func TestParseProxy(t *testing.T) { | |||
| func withEnvVars(t *testing.T, newEnvVars map[string]string, f func()) { | |||
There was a problem hiding this comment.
Minor: the restore now runs in t.Cleanup, so the f func() parameter only adds nesting. The closure also no longer marks where the variables stop applying. Code after withEnvVars in the same test, or a second withEnvVars call, sees the first call's variables until the test ends. No current caller does this.
A setEnvVars(t, env) helper without the closure would match the new behavior. t.Setenv might also work now. The "causes errors on Windows for some reason" note on the nolint comments might be out of date.
Claude Code (an AI agent) wrote this comment for @oschwald.
| // path. It then returns the file contents, with leading and trailing white | ||
| // space removed, and true. If nothing remains, it returns an error. The file | ||
| // takes precedence over the variable named by key. | ||
| func lookupEnvOrFile(key string) (string, bool, error) { |
There was a problem hiding this comment.
Minor: the name lookupEnvOrFile suggests "variable first, then file", but the file takes precedence. A reader who sees only the call sites can get the order wrong. lookupFileOrEnv would match the behavior.
The PR description says that the name follows os.LookupEnv, so keeping it is also fine.
Claude Code (an AI agent) wrote this comment for @oschwald.
There was a problem hiding this comment.
It is true. I would have thought this.
| Err: "GEOIPUPDATE_LICENSE_KEY_FILE points to an empty file: " + blankFile, | ||
| }, | ||
| { | ||
| Description: "Blank PROXY_USER_PASSWORD_FILE", |
There was a problem hiding this comment.
No test checks 2 behaviors that commit 99ac625 describes:
GEOIPUPDATE_PROXY_USER_PASSWORD=""still clearsProxyUserPasswordfromGeoIP.conf.- A value from
GEOIPUPDATE_PROXY_USER_PASSWORD_FILEreachesConfig.Proxy.User.
A NewConfig test case with ProxyUserPassword in the config file, plus each environment form, would cover both.
Claude Code (an AI agent) wrote this comment for @oschwald.
There was a problem hiding this comment.
Added in 0bcb6c4. 2 new TestNewConfig cases have ProxyUserPassword in the config file:
- One sets
GEOIPUPDATE_PROXY_USER_PASSWORD=""and expects no user inConfig.Proxy. - One sets
GEOIPUPDATE_PROXY_USER_PASSWORD_FILEand expects the user and password from the file inConfig.Proxy.User.
Claude Code (an AI agent) wrote this reply for @horgh.
| // Use Cleanup so that the environment is restored even if f fails the | ||
| // test. |
There was a problem hiding this comment.
Nit: active voice reads more directly here.
| // Use Cleanup so that the environment is restored even if f fails the | |
| // test. | |
| // Cleanup restores the environment even if f fails the test. |
Claude Code (an AI agent) wrote this comment for @oschwald.
withEnvVars set newEnvVars on top of the current environment. A GEOIPUPDATE_* variable from the shell stayed set, so the result of the tests depended on the shell. With GEOIPUPDATE_EDITION_IDS and GEOIPUPDATE_PROXY_USER_PASSWORD set, 27 subtests failed. withEnvVars now unsets the GEOIPUPDATE_* variables before it sets newEnvVars. The cleanup restores them. Co-Authored-By: Claude Opus 5.5 <[email protected]>
The cleanup in withEnvVars cleared the whole environment and then set each original variable again. This had 2 problems: - If one os.Setenv call failed, require.NoError stopped the loop. The remaining variables, for example PATH and HOME, did not come back, and later tests ran with an almost empty environment. - On Windows, os.Environ can return names that start with "=", for example "=C:=C:\src". strings.SplitN gives an empty name for such a pair, and os.Setenv returns an error for it. withEnvVars now uses t.Setenv. t.Setenv restores only the variables that it changed, so the environment is not cleared. The old comment said that t.Setenv caused errors on Windows. It was used in the restore loop, after os.Clearenv, so it recorded each variable as not set and removed all of them when the test ended. That does not occur now. Co-Authored-By: Claude Opus 5.5 <[email protected]>
geoipupdate removes leading and trailing white space from the file that a _FILE variable names, but the docs did not say so. For GEOIPUPDATE_PROXY_USER_PASSWORD_FILE this can change the password. A file that contains "user:pass " gives the password "pass", but the same value in GEOIPUPDATE_PROXY_USER_PASSWORD gives "pass ". Co-Authored-By: Claude Opus 5.5 <[email protected]>
The error said "points to an empty file", but it also occurs for a file that contains only white space, for example a single newline. Such a file does not show as empty in "ls -l". The error also did not quote the path, so a path with a trailing space looked correct. The error now says that the file is empty or contains only white space, and it quotes the path. Co-Authored-By: Claude Opus 5.5 <[email protected]>
The docs said that the _FILE variable takes precedence if both variables are set. But geoipupdate ignores a _FILE variable that is set to an empty string. With GEOIPUPDATE_LICENSE_KEY=abc and GEOIPUPDATE_LICENSE_KEY_FILE set but empty, geoipupdate uses abc. The docs now say that the _FILE variable takes precedence if it is set and not empty. They also say that an empty or white-space-only file causes an error. Co-Authored-By: Claude Opus 5.5 <[email protected]>
The docker docs said "a variable and its _FILE form". A user can read this to mean that every variable has a _FILE form, for example GEOIPUPDATE_EDITION_IDS_FILE. geoipupdate ignores that variable. Also, GEOIPUPDATE_CONF_FILE and GEOIPUPDATE_LOCK_FILE end in _FILE but are not file forms of other variables. The docs now name the 3 variables that have a _FILE form. Co-Authored-By: Claude Opus 5.5 <[email protected]>
withEnvVars ran the test in a closure. The closure no longer marks where the variables apply, because t.Setenv restores them only when the test ends. Code after the closure still saw the variables. setEnvVars sets the variables and returns. The variables apply until the test ends. Co-Authored-By: Claude Opus 5.5 <[email protected]>
The name lookupEnvOrFile suggests that the function checks the variable first and then the file. The file takes precedence, so lookupFileOrEnv matches the behavior. Co-Authored-By: Claude Opus 5.5 <[email protected]>
No test checked 2 behaviors that NewConfig must keep: - GEOIPUPDATE_PROXY_USER_PASSWORD="" removes the user and password that ProxyUserPassword sets in GeoIP.conf. - The value from GEOIPUPDATE_PROXY_USER_PASSWORD_FILE gets to Config.Proxy.User. Co-Authored-By: Claude Opus 5.5 <[email protected]>
This cleans up #570, which added
GEOIPUPDATE_PROXY_USER_PASSWORD_FILE. Each commit makes one small change, so it is easiest to review one commit at a time.Code:
_FILEhelper now checkerrbeforefound.lookupEnvOrFile, followsos.LookupEnv.Tests:
GEOIPUPDATE_ACCOUNT_IDis ignored whenGEOIPUPDATE_ACCOUNT_ID_FILEis set. Before Allow declaring a file containing the proxy userinfo via the environment #570, it caused an error._FILEvariable returns an error when its file is missing._FILEvariables work without their base variables.PROXY_USER_PASSWORD_FILE, which is what they test.Docs:
doc/docker.mdanddoc/GeoIP.conf.mdlistGEOIPUPDATE_PROXY_USER_PASSWORD_FILE. TheGeoIP.conf(5)man page is built fromdoc/GeoIP.conf.md._FILEform takes precedence when both variables are set.CHANGELOG.mdhas an entry for the new variable.🤖 Generated with Claude Code
Summary by CodeRabbit
_FILEoption are set, the file value takes precedence.