adam's branch. delete this.#96
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds a new customResourcesMap configuration option to the Helm chart, allowing users to define custom Kubernetes resources using a map structure in addition to the existing list-based customResources option. The map-based approach facilitates merging custom resources from multiple values files. The PR includes version bump to 0.10.0-rc1, comprehensive test coverage, example documentation, and updates the template logic to handle both resource definition styles.
Changes:
- Added
customResourcesMapconfiguration option for defining custom resources as key-value pairs - Updated custom-resources template to merge and process resources from both
customResourceslist andcustomResourcesMapdictionary - Version bumped from 0.9.0 to 0.10.0-rc1
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Chart.yaml | Version bumped from 0.9.0 to 0.10.0-rc1 |
| values.yaml | Added customResourcesMap configuration with inline documentation and example |
| templates/custom-resources.yaml | Updated template logic to merge resources from both customResources list and customResourcesMap dictionary |
| tests/custom-resources_test.yaml | Added comprehensive test suite covering both configuration options, templating, and edge cases |
| examples/customresources/values.yaml | Added example demonstrating both customResources and customResourcesMap usage patterns |
| README.md | Updated documentation table to include customResourcesMap and version badge |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| suite: custom-resources | ||
|
|
||
| templates: | ||
| - custom-resources.yaml |
There was a problem hiding this comment.
The template path should include the 'templates/' prefix for consistency with the majority of test files in this repository. Most test files (configmap_test.yaml, deployment_test.yaml, externalsecret_test.yaml, ingress_test.yaml, keda_scaledobject_test.yaml, keda_triggerauthentication_test.yaml, pdb_test.yaml, service_test.yaml, serviceaccount_test.yaml, statefulset_test.yaml) use 'templates/filename.yaml' format. Change 'custom-resources.yaml' to 'templates/custom-resources.yaml'.
| - custom-resources.yaml | |
| - templates/custom-resources.yaml |
| {{- $resources1 := default (list) .Values.customResources }} | ||
| {{- $resources2 := values (default (dict) .Values.customResourcesMap) }} | ||
| {{- range concat $resources1 $resources2 }} | ||
| {{- with (tpl . $ | fromYaml ) }} |
There was a problem hiding this comment.
Remove the space before the closing parenthesis in the with statement. The codebase consistently uses no space before closing parentheses, as seen in other template files (e.g., templates/_podTemplate.tpl lines 69, 73, 129, 319 use (toYaml .) not (toYaml . )). Change fromYaml ) to fromYaml).
| {{- with (tpl . $ | fromYaml ) }} | |
| {{- with (tpl . $ | fromYaml) }} |
a728622 to
c391f89
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {{- range concat $resources1 $resources2 }} | ||
| {{- /* | ||
| There can be multiple documents in the rendered output of a single template, | ||
| so we want to split thing so each of those becomes its own entry. |
There was a problem hiding this comment.
Spelling error in comment: "thing" should be "things".
| so we want to split thing so each of those becomes its own entry. | |
| so we want to split things so each of those becomes its own entry. |
| apiVersion: v2 | ||
| description: A Helm chart template for applications | ||
| version: 0.9.0 | ||
| version: 0.10.0-rc2 |
There was a problem hiding this comment.
The PR title "adam's branch. delete this." suggests this is a temporary branch not intended for merging, but the changes appear to be a legitimate and well-implemented feature addition. Please clarify the intent of this PR - if these changes are intended to be merged, update the PR title and description to properly describe the feature being added.
| @@ -0,0 +1,320 @@ | |||
| suite: custom-resources | |||
There was a problem hiding this comment.
The suite name should follow the naming convention used in other test files. Most test files use CamelCase or PascalCase for suite names (e.g., "Deployment", "ConfigMap", "ServiceAccount", "PodDisruptionBudget"). Consider changing "custom-resources" to "CustomResources" for consistency.
| suite: custom-resources | |
| suite: CustomResources |
No description provided.