[cozy-lib] Improve flatten function - #1647
Conversation
This patch breaks introduces a helper function in cozy-lib to correctly handle special case resources when transforming a nested map of limits and requests to a flat map suitable for use in resourceQuotas. As a result, admins can now specify any types of resources as resource quotas for tenants, and they will be correctly transformed to the correct format for the underlying kubernetes ResourceQuota. In addition to the previously supported compute resources, such as CPU, memory, and custom resources, like GPUs, special quota strings such as "services.loadbalancers" are now correctly handled. ```release-note [cozy-lib,platform] Support resource quotas for special kubernetes quotas, such as service.loadbalncer count and others. ``` Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
Summary of ChangesHello Timofei Larkin (@lllamnyp), I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the resource flattening logic within Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request significantly improves the cozy-lib.resources.flatten helper by refactoring it to correctly handle special Kubernetes resource quota types alongside standard compute resources. The introduction of the cozy-lib.resources.flattenResource helper centralizes the transformation logic, making the implementation much cleaner and more robust. The accompanying test changes correctly validate the new functionality.
My review includes a few suggestions for the new cozy-lib.resources.flattenResource helper to enhance its correctness and maintainability. I've pointed out a potential typo in a resource key and recommended expanding the documentation and refactoring the conditional logic to be more explicit. Overall, this is a great enhancement.
| "services" | ||
| "services.loadbalancers" | ||
| "services.nodeports" | ||
| "services.clusterip" |
There was a problem hiding this comment.
It appears there might be a typo here. According to the Kubernetes documentation, the correct resource quota key for ClusterIP services is services.clusterips (plural), not services.clusterip (singular). Using the singular form will likely result in the quota not being applied correctly.
"services.clusterips"
| This is a helper function that takes an argument like `list "limits" "services.loadbalancers"` | ||
| or `list "limits" "storage"` or `list "requests" "cpu"` and returns "services.loadbalancers", | ||
| "", and "requests.cpu", respectively, thus transforming them to an acceptable format for k8s | ||
| ResourceQuotas objects. | ||
| */}} |
There was a problem hiding this comment.
The comment provides good examples, but for better long-term maintainability, it would be beneficial to also document the general rules being applied for the transformation. This would make the logic in the template easier to understand without needing to trace the code.
Consider adding a brief explanation of the rules, for example:
- Standard compute and custom resources (e.g.,
cpu,memory) are prefixed withrequests.orlimits.. - Raw Kubernetes object count quotas (e.g.,
pods,services.loadbalancers) are treated as limits; thelimits.prefix is removed, and anyrequests.version is dropped. limits.storageis explicitly dropped as it's not a valid resource quota key, whilerequests.storageis preserved.
This helper function transforms sanitized resource sections and types into the final key format for a Kubernetes ResourceQuota.
The transformation rules are as follows:
- Standard compute/custom resources (e.g., `cpu`, `memory`) are prefixed with `requests.` or `limits.`.
- Raw object count quotas (e.g., `pods`, `services.loadbalancers`) are treated as limits; the `limits.` prefix is removed, and any `requests.` version is dropped.
- `limits.storage` is explicitly dropped as it's not a valid key, while `requests.storage` is preserved.
| {{- $out := "" }} | ||
| {{- if and (eq $section "limits") (eq $type "storage") }} | ||
| {{- $out = "" }} | ||
| {{- else if and (eq $section "limits") (has $type $rawQuotaKeys) }} | ||
| {{- $out = $type }} | ||
| {{- else if not (has $type $rawQuotaKeys) }} | ||
| {{- $out = printf "%s.%s" $section $type }} | ||
| {{- end }} | ||
| {{- $out -}} |
There was a problem hiding this comment.
The current logic for flattening resource keys is correct, but its reliance on fall-through for certain cases (like requests for a raw quota key) makes it less explicit and harder to reason about. To improve clarity and maintainability, consider restructuring the conditional logic to explicitly handle all cases, including those that result in an empty string. This makes the function's behavior more self-documenting.
{{- $out := "" }}
{{- if and (eq $section "requests") (has $type $rawQuotaKeys) }}
{{- /* Raw quota keys are counts and do not have a 'requests' counterpart. */}}
{{- else if and (eq $section "limits") (eq $type "storage") }}
{{- /* 'limits.storage' is not a valid resource quota key. */}}
{{- else if and (eq $section "limits") (has $type $rawQuotaKeys) }}
{{- $out = $type }}
{{- else if not (has $type $rawQuotaKeys) }}
{{- $out = printf "%s.%s" $section $type }}
{{- end }}
{{- $out -}}
This patch breaks introduces a helper function in cozy-lib to correctly handle special case resources when transforming a nested map of limits and requests to a flat map suitable for use in resourceQuotas. As a result, admins can now specify any types of resources as resource quotas for tenants, and they will be correctly transformed to the correct format for the underlying kubernetes ResourceQuota. In addition to the previously supported compute resources, such as CPU, memory, and custom resources, like GPUs, special quota strings such as "services.loadbalancers" are now correctly handled.
What this PR does
Release note