Conversation
|
@whs-dot-hk would you be able to describe the problem this PR is meant to address? Thanks! |
There was a problem hiding this comment.
Pull request overview
This PR refactors how Kubernetes complex list types with merge keys are handled in the generator. The main change simplifies the mergeValuesByKey function to always construct attribute names by concatenating values from all relevant keys (mergeKey + listMapKeys), removing the previous conditional logic that used special prefixes for generated keys.
Key changes:
- Simplified mergeValuesByKey function to always use all keys for naming
- Renamed parameters for clarity (attrMergeKey → mergeKey, listMergeKeys → listMapKeys)
- Updated generated modules to include mergeKey in listMapKeys where appropriate (e.g., ContainerPort now uses "containerPort" as mergeKey with ["containerPort", "protocol"] as listMapKeys)
Reviewed changes
Copilot reviewed 2 out of 11 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/k8s/defaults.nix | Adds test case for container ports with different protocols to validate the fix |
| pkgs/generators/k8s/default.nix | Refactors mergeValuesByKey and related functions, removes conditional logic |
| modules/generated/*.nix | Updates all generated modules across Kubernetes versions 1.19-1.27 with new merge key configurations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| (concatStringsSep "" (map (key: | ||
| if value ? ''${key} | ||
| then | ||
| if isAttrs value.''${key} | ||
| then toString value.''${key}.content | ||
| else (toString value.''${key}) | ||
| else "" |
There was a problem hiding this comment.
The mergeValuesByKey function generates attribute names by concatenating values from all keys (mergeKey + listMapKeys). When a key is missing from a value, an empty string is concatenated. This could lead to collisions if multiple list items have the same subset of keys defined. For example, if mergeKey is "port" and listMapKeys is ["port", "protocol"], an item with only port=80 would generate the same key "80" as another item with port=80 and protocol="" (empty string), causing one to overwrite the other.
| (concatStringsSep "" (map (key: | |
| if value ? ''${key} | |
| then | |
| if isAttrs value.''${key} | |
| then toString value.''${key}.content | |
| else (toString value.''${key}) | |
| else "" | |
| (concatStringsSep ":" (map (key: | |
| let | |
| hasKey = builtins.hasAttr key value; | |
| in | |
| if hasKey then | |
| let | |
| v = value.${key}; | |
| in | |
| if isAttrs v | |
| then "${key}=${toString v.content}" | |
| else "${key}=${toString v}" | |
| else "${key}=<missing>" |
No description provided.