feat: overrides config with environment variables#16
Conversation
This patch adds support for overriding selected config params with environment variables. If unset, they are loaded with sane default values. Signed-off-by: Juliana Oliveira <juliana@fly.io>
jwnx
force-pushed
the
jwnx/fix-increase-etcd-max-storage-to-5gib
branch
from
d306d8f to
61e6813
Compare
jwnx
changed the title
jphenow
left a comment
jphenow
left a comment
There was a problem hiding this comment.
might be worth adding a test or two but not a hill I'd die on - it's pretty straight-forward looking
internal/flyetcd/node.go
Outdated
| cfg, err := loadConfig() | ||
| // NewConfig provides sane defaults for fields that may not be present in | ||
| // older config files. The config file will overlay on top of these defaults. | ||
| cfg, err := NewConfig() |
There was a problem hiding this comment.
Notably NewConfig() calls SetAuthToken(), which looks to have side effects — it creates a certs/ directory and writes JWT key files to disk.
This runs every boot now instead of just on missing config - idk if we care about that it just is.
There was a problem hiding this comment.
Running on every boot is not a problem, IMO. But I fixed the order a bit to make precedence clearer.
Now, reload will:
- fetch default values (no side effect)
- overlay config file (if present)
- overlay env vars (if present)
- finally, call SetAuthToken (always does it)
internal/flyetcd/config.go
Outdated
| MaxWals: 10, | ||
| SnapshotCount: 10000, // Default | ||
| SnapshotCount: 10000, // Default | ||
| QuotaBackendBytes: 2147483648, // 2GiB |
There was a problem hiding this comment.
maybe overkill but I wonder if we should have it match size of the available volume?
There was a problem hiding this comment.
and prefer the value passed in via env or config or whatever
There was a problem hiding this comment.
nit: I prefer the 2 * 1024 * 1024 * 1024, // 2GiB style but I don't care that much
internal/flyetcd/config.go
Outdated
| return fallback | ||
| } | ||
| result = n | ||
| case bool: |
There was a problem hiding this comment.
this seems to be unused - could remove maybe
There was a problem hiding this comment.
Removed (I started with big plans for this function and then decided we should leave for some other time)
| @@ -36,9 +37,10 @@ type Config struct { | |||
| AutoCompactionRetention string `yaml:"auto-compaction-retention"` | |||
| AuthToken string `yaml:"auth-token"` | |||
There was a problem hiding this comment.
worth adding var handling for the aboves as well?
There was a problem hiding this comment.
Perhaps separate PR if it matters. we have a more specific need right now
There was a problem hiding this comment.
Added for AutoCompactionMode and AutoCompactionRetention, they could prove useful (:
2 participants
This patch adds support for overriding selected config params with
environment variables. If unset, they are loaded with sane default
values.
Signed-off-by: Juliana Oliveira juliana@fly.io