Enable environment overrides and built-in configuration defaults #963
Conversation
internal/config/auth.go
Outdated
| // NoncePeriod is the period in seconds for which a nonce is valid | ||
| NoncePeriod int64 `mapstructure:"nonce_period"` | ||
| NoncePeriod int64 `mapstructure:"nonce_period" default:"60"` |
There was a problem hiding this comment.
NoncePeriod is unused as of now correct? I was wondering because this new default is quite a bit lower than the original one, but git grep -I nonceperiod only finds these two lines..
There was a problem hiding this comment.
Thanks, I zoned out on this one and assumed that copilot had pulled out other correct values, it had the correct one here. FIxed.
| v.SetDefault("logging.level", "debug") | ||
| v.SetDefault("logging.format", "json") | ||
| v.SetDefault("logging.logFile", "") | ||
| Level string `mapstructure:"level" default:"debug"` |
There was a problem hiding this comment.
I know you didn't change this default, but do you think the default log level should be debug?
There was a problem hiding this comment.
🤷
I'm fine with changing it, but the goal here was to make it so that the code actually had all the defaults.
There was a problem hiding this comment.
Not in this PR, but I was wondering if you had some thoughts about how verbose should the default deployment be based on your SaaS work.
There was a problem hiding this comment.
I figured we'd set the defaults to be appropriate for development work, though I could see preferring info for that as well to avoid things being too spammy.
| @@ -26,13 +26,13 @@ type AuthConfig struct { | |||
| // RefreshTokenPublicKey is the public key used to verify the refresh token for authn/z | |||
| RefreshTokenPublicKey string `mapstructure:"refresh_token_public_key"` | |||
| // TokenExpiry is the expiry time for the access token in seconds | |||
| TokenExpiry int64 `mapstructure:"token_expiry"` | |||
| TokenExpiry int64 `mapstructure:"token_expiry" default:"3600"` | |||
There was a problem hiding this comment.
Do you think the other fields could also use defaults?
There was a problem hiding this comment.
Huh, I somehow lost just that one file in my commit. I suspect I hadn't saved the buffer.
Fixed!
| } | ||
|
|
||
| // GetCryptoConfigWithDefaults returns a CryptoConfig with default values | ||
| // TODO: extract from struct default tags | ||
| func GetCryptoConfigWithDefaults() CryptoConfig { |
There was a problem hiding this comment.
It might be nice to rename this function in a future patch, currently it's used only in tests and returns DefaultConfigForTest, but the name sounds like non-test function.
There was a problem hiding this comment.
Ooops, I do extract this from struct tags now, and you're right, I should either remove this and have the tests use DefaultConfigForTest or name this ForTest.
| v.SetDefault("logging.level", "debug") | ||
| v.SetDefault("logging.format", "json") | ||
| v.SetDefault("logging.logFile", "") | ||
| Level string `mapstructure:"level" default:"debug"` |
There was a problem hiding this comment.
Not in this PR, but I was wondering if you had some thoughts about how verbose should the default deployment be based on your SaaS work.
I spent a bit of time trying to get the env vars to work and I ended up reworking defaults as well.