loadConfig: let OCGO_API_KEY override the config file, as documented - #15
Open
quadseven wants to merge 1 commit into
Open
loadConfig: let OCGO_API_KEY override the config file, as documented#15quadseven wants to merge 1 commit into
quadseven wants to merge 1 commit into
Conversation
OCGO_API_KEY was seeded as the struct default BEFORE json.Unmarshal read config.json over it, so a stale `api_key` in the file silently won. The README says "You can also provide the key at runtime with an environment variable", and it does not work whenever a config file exists. The failure is quiet and expensive to chase: every request comes back `401 Invalid API key` with no indication which credential was used, while `ocgo setup` looks fine and the documented override appears to be ignored for no reason. Reading the file first and letting a non-empty env var win afterwards matches the documentation and the usual precedence (explicit env > file > defaults). It also makes it possible to source the key from a secret store at run time without ever writing it to disk. Whitespace-only values are treated as unset, so an `export OCGO_API_KEY=` left in a shell profile cannot blank out a working config file.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
OCGO_API_KEYis seeded as the struct default beforejson.Unmarshalreadsconfig.jsonover it:So a stale
api_keyin the file silently wins, and the env var documented in the README —— does nothing whenever a config file exists.
Hit this for real: a stale key in
config.json, the correct key exported inOCGO_API_KEY, and every request returning401 Invalid API keythrough bothocgo serveand direct calls. Nothing indicates which credential was used,ocgo setuplooks healthy, and the documented override appears to be ignored for no reason.What
Read the file first, then let a non-empty env var win —
explicit env > config file > defaults, matching the README and the usual precedence.TrimSpacemeans a whitespace-only value counts as unset, so anexport OCGO_API_KEY=left in a shell profile cannot blank out a working config file.Side benefit: it makes it possible to source the key from a secret store at run time without ever writing it to disk, which is why I went looking.
Test plan
Three new tests.
configFile()resolves throughos.UserHomeDir(), so they pointHOMEat a temp dir witht.Setenvrather than needing production code changed for testability:TestEnvKeyOverridesConfigFile— env wins over a differing file valueTestConfigFileUsedWhenEnvUnset— file still used when no env varTestBlankEnvDoesNotClobberConfigFile— whitespace-only env ignoredVerified the first one actually catches the bug by restoring the original ordering:
Restored → all pass.
go build ./...,go vet ./...,go test ./...andgofmt -l .all clean.