Use kebab-case property keys for env-var-overridable config properties - #1096
Conversation
d0e5bc2 to
8c6a75c
Compare
| private boolean debug | ||
|
|
||
| @Value('${wave.build.k8s.storage.claimName}') | ||
| @Value('${wave.build.k8s.storage.claim-name:}') |
There was a problem hiding this comment.
I am not sure we want to keep this empty defaults instead of using null when not set. That might be beyond the scope of this PR though
|
The kebab-case rename itself is safe — Micronaut normalizes every property-source key to kebab ( 1. Revert the behavior change in @Value('${wave.build.k8s.storage.claim-name:}') // ':' added
@Value('${wave.build.k8s.storage.mount-path:}')The 2. Revert the release bump. The 3. Follow-up: align the wave config in platform-deployment. Not required (camelCase still binds), but for consistency we should also switch the mounted
I'll open that as a separate platform-deployment PR alongside the version bump. |
|
if you can fix up the small nits around empty defaults we can include this in the changelog. I think we should update the documentation as we cannot promise MN5/6 will keep this behaviour in the longterm. |
Several Wave config properties used a camelCase segment in their name (e.g. wave.build.k8s.storage.claimName, wave.blobCache.storage.accessKey, wave.httpclient.connectTimeout). Such a property cannot be populated from an environment variable via Micronaut's implicit ENV_VAR fold: the uppercased env var loses the word boundary, so WAVE_..._CLAIMNAME folds to the candidate `claimname`, which never matches the property's normalized key `claim-name`. As a result the value silently bound to null and could only be set from a config file, not an env var. Switch the @value / @Property (and the corresponding @requires) keys to kebab-case. Micronaut normalizes every property-source key to kebab (NameUtils.hyphenate), so this is a pure alias swap: existing config using `claimName` / `mountPath` / `blobCache.*` keeps binding unchanged, while the kebab key also folds correctly from the matching environment variable (WAVE_BUILD_K8S_STORAGE_CLAIM_NAME, WAVE_BLOB_CACHE_STORAGE_ACCESS_KEY, WAVE_HTTPCLIENT_CONNECT_TIMEOUT, ...). The change is kept in the @value annotations rather than declared in the shared application.yml: for the k8s.* keys, declaring any wave.build.k8s.* value in the base config would satisfy the @requires(property = 'wave.build.k8s') gate on K8sServiceImpl (and the other Kube* beans) in every environment, forcing them to instantiate where wave.build.k8s.namespace is not set. Keys converted (property -> env var): - wave.build.k8s.storage.claim-name WAVE_BUILD_K8S_STORAGE_CLAIM_NAME - wave.build.k8s.storage.mount-path WAVE_BUILD_K8S_STORAGE_MOUNT_PATH - wave.build.k8s.config-path WAVE_BUILD_K8S_CONFIG_PATH - wave.build.logs.max-length WAVE_BUILD_LOGS_MAX_LENGTH - wave.allow-anonymous WAVE_ALLOW_ANONYMOUS - wave.deny-paths / wave.deny-hosts WAVE_DENY_PATHS / WAVE_DENY_HOSTS - wave.close-session-on-invalid-license-token - wave.pairing.channel.await-timeout / max-attempts / retry-back-off-base / retry-back-off-delay / retry-max-delay - wave.httpclient.connect-timeout / stream-threshold / retry.max-delay - wave.aws.sts.retry.max-delay - wave.mirror.skopeo-image - wave.cache.digest-store.max-weight-mb - wave.blob-cache.* (status, storage.access-key/secret-key/endpoint, base-url, s5cmd-image, k8s.resources.*, cloudflare.*) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
8c6a75c to
2d6ba3a
Compare
pditommaso
left a comment
There was a problem hiding this comment.
Thanks @bebosudo, this looks great now — the earlier nits are all sorted and the rename is a clean kebab-case alias swap. Approving! 🎉
Leaving the actual merge to @gavinelder, since his docs request (we can't promise MN5/6 keeps this fold behaviour long-term) looks still unresolved.
Problem
Several Wave config properties use a camelCase segment in their name — e.g.
wave.build.k8s.storage.claimName,wave.blobCache.storage.accessKey,wave.httpclient.connectTimeout. Such a property cannot be populated from an environment variable via Micronaut's implicitENV_VARfold: the uppercased env var loses the word boundary, soWAVE_BUILD_K8S_STORAGE_CLAIMNAMEfolds to the candidateclaimname, which never matches the property's normalized keyclaim-name. The value silently binds tonulland can only be set from a config file, not an env var.This was hit in practice:
WAVE_BUILD_K8S_STORAGE_CLAIMNAME/..._MOUNTPATHhad no effect, andstorageClaimName/storageMountPathlogged asnull.Fix
Switch the affected
@Value/@Property(and the corresponding@Requires) keys to kebab-case. Micronaut resolves a kebab key against both the kebab and the legacy camelCase spelling, so:claimName/mountPath/blobCache.*still bind unchanged (kebab reads camelCase).WAVE_BUILD_K8S_STORAGE_CLAIM_NAME,WAVE_BLOB_CACHE_STORAGE_ACCESS_KEY,WAVE_HTTPCLIENT_CONNECT_TIMEOUT, …Why at the
@Valuesource, not inapplication.ymlFor the
wave.build.k8s.*keys, declaring any such value in the shared base config would satisfy the@Requires(property = 'wave.build.k8s')gate onK8sServiceImpl(and the otherKube*beans) in every environment, forcing them to instantiate wherewave.build.k8s.namespaceis not set (this broke 121 tests in an earlier attempt). Keeping the change in the annotation avoids tripping the gate. The empty default on the nullable k8s storage fields keeps them optional without introducing the property into shared config.Keys converted (property → env var)
wave.build.k8s.storage.claim-nameWAVE_BUILD_K8S_STORAGE_CLAIM_NAMEwave.build.k8s.storage.mount-pathWAVE_BUILD_K8S_STORAGE_MOUNT_PATHwave.build.k8s.config-pathWAVE_BUILD_K8S_CONFIG_PATHwave.build.logs.max-lengthWAVE_BUILD_LOGS_MAX_LENGTHwave.allow-anonymousWAVE_ALLOW_ANONYMOUSwave.deny-paths/wave.deny-hostsWAVE_DENY_PATHS/WAVE_DENY_HOSTSwave.close-session-on-invalid-license-tokenWAVE_CLOSE_SESSION_ON_INVALID_LICENSE_TOKENwave.pairing.channel.await-timeout/max-attempts/retry-back-off-base/retry-back-off-delay/retry-max-delayWAVE_PAIRING_CHANNEL_*wave.httpclient.connect-timeout/stream-threshold/retry.max-delayWAVE_HTTPCLIENT_*wave.aws.sts.retry.max-delayWAVE_AWS_STS_RETRY_MAX_DELAYwave.mirror.skopeo-imageWAVE_MIRROR_SKOPEO_IMAGEwave.cache.digest-store.max-weight-mbWAVE_CACHE_DIGEST_STORE_MAX_WEIGHT_MBwave.blob-cache.*(status, storage.access-key/secret-key/endpoint, base-url, s5cmd-image, k8s.resources., cloudflare.)WAVE_BLOB_CACHE_*Compatibility
No config contract is broken: the camelCase spelling remains an accepted alias, so existing
application.yml/config.ymldeployments and the docs' examples continue to work. The kebab spelling is simply also accepted and is the form that folds from env vars.Verification
@Valuekey resolves both spellings, that empty-string defaults coerce tonullfor typed scalars, that a bare${VAR}with no default fails startup (avoided), and that an empty@Valuedefault does not trip a@Requires(property=...)gate.RegistryAuthServiceTestetc.), confirmed to fail identically onmaster.🤖 Generated with Claude Code