fix ConfigMap data with leading underscore (#44) #45
Conversation
|
I believe attrs starting with _ are filtered due to some specific “special” attrs which are use throughout the module system. See: https://github.com/search?q=repo%3Ahall%2Fkubenix+._&type=code It might be better to compile a list of these and only exclude this specific list (everywhere). |
There was a problem hiding this comment.
I agree with @adrian-gierakowski that it's probably cleaner to not remove everything beginning with an underscore but instead to remove specifically those keys (from all objects, not just 1 or 2 fields of 1 object) which are not provided by the user.
However, like you, I'm also skeptical why this needs to be done. My guess would be the same as Adrian's but the tests don't fail on main when removing && !(hasPrefix "_" n) and that the test you've added here (which, at a glance, should catch any keys leaked from the module system) doesn't fail without any other change to the k8s module. So it would seem that removing the leading underscore isn't necessary (it very well could've been added while under heavy development then things changed and it was no longer necessary but not removed 🤷). I haven't tested that guess on an actual cluster though.
| # => To get a minimal invasive fix we added `objType` and `propertyPath` to make `moduleToAttrs` "context-aware". | ||
| # This way we can allow names with leading underscore exactly for ConfigMap -> data/binaryData | ||
| allowLeadingUnderscore = objType.group == "core" && objType.version == "v1" && objType.kind == "ConfigMap" && | ||
| (propertyPath == [ "data" ] || propertyPath == [ "binaryData" ]); |
There was a problem hiding this comment.
The underscore isn't a valid base64 character so we shouldn't need to check binaryData, yeah?
| @@ -21,6 +21,7 @@ let | |||
| ./k8s/defaults.nix | |||
| ./k8s/order.nix | |||
| ./k8s/submodule.nix | |||
| ./k8s/configmap.nix | |||
| moduleToAttrs = objType: propertyPath: value: | ||
| if isAttrs value then | ||
| let | ||
| # Fix https://github.com/hall/kubenix/issues/44 |
There was a problem hiding this comment.
nit: I'd argue most of this should comment probably go in the commit message details but I'd rather too many comments than too few so don't let that stop you 🙂
felixscheinost
force-pushed
the
feature/fix-leading-underscore-configmap
branch
from
20c66c2
to
242a48d
Compare
I tried doing the least amount of changes to fix keys with leading underscore in ConfigMap data.