Handle valuesFiles from cozypkg.cozystack.io/values-files annotation #8
Conversation
Signed-off-by: Andrei Kvapil <[email protected]>
|
Caution Review failedThe pull request is closed. WalkthroughThe change shifts chart metadata resolution from HelmRelease.chart/ChartRef to reading local Chart.yaml files. Annotation-based ExternalArtifact value file resolution replaces chartRef pathways. Multiple helper functions gain a chartDir parameter, with propagation through call sites and command flow paths. Changes
Sequence DiagramsequenceDiagram
participant Command as Command Flow
participant GetChart as getChartInfo()
participant Merged as mergedValues()
participant Render as renderManifests()
participant History as newHistoryEntry()
participant Status as markSuccess()
participant LocalChart as Local Chart.yaml
participant HR as HelmRelease Annotations
Command->>GetChart: chartDir
GetChart->>LocalChart: Read Chart.yaml
LocalChart-->>GetChart: Chart name, version
Command->>Merged: chartDir, HelmRelease
Merged->>HR: Check annotation (values-files)
HR-->>Merged: Annotation or fallback to spec
Merged-->>Merged: Parse, trim, load from disk
Command->>Render: chartDir, mergedValues
Render-->>Command: Rendered manifests
Command->>History: chartDir, status
History->>GetChart: chartDir
GetChart->>LocalChart: Extract chart name
LocalChart-->>History: Chart metadata
Command->>Status: chartDir, success
Status->>GetChart: chartDir
GetChart-->>Status: Chart info (fallback to HR name on error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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 |
Summary of ChangesHello @kvaps, 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 introduces a significant enhancement to how Helm 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-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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors how chart information and valuesFiles are handled. The getChartInfo function is updated to read from a local Chart.yaml, which simplifies the logic and aligns with the tool's local development focus. The most significant change is the introduction of a new annotation, cozypkg.cozystack.io/values-files, to specify values files, replacing the previous method of resolving them via ChartRef. The implementation is mostly solid, but I've identified a bug in the parsing logic for the new annotation that could cause the program to fail. My review includes a suggested fix for this issue.
| if vfStr != "" { | ||
| valuesFiles = strings.Split(vfStr, ",") | ||
| // Trim whitespace from each value | ||
| for i, vf := range valuesFiles { | ||
| valuesFiles[i] = strings.TrimSpace(vf) | ||
| } | ||
| } |
There was a problem hiding this comment.
The current logic for parsing the cozypkg.cozystack.io/values-files annotation has a bug. If the annotation value contains extra commas or whitespace-only parts (e.g., "file1.yaml, , file2.yaml"), the resulting valuesFiles slice will contain empty strings. This will later cause os.ReadFile to fail when it tries to read a directory path instead of a file.
To fix this, you should filter out empty strings after splitting and trimming the annotation value.
Additionally, it's good practice to define the annotation key "cozypkg.cozystack.io/values-files" as a constant at the package level to improve maintainability and avoid typos.
if vfStr != "" {
for _, vf := range strings.Split(vfStr, ",") {
trimmed := strings.TrimSpace(vf)
if trimmed != "" {
valuesFiles = append(valuesFiles, trimmed)
}
}
}
Signed-off-by: Andrei Kvapil [email protected]
Summary by CodeRabbit