Use go embed instead of code generation #79
Conversation
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 modernizes the application's handling of embedded chart files by transitioning from a custom code generation approach to Go's built-in 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
|
|
Warning Rate limit exceeded@kvaps has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 21 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRefactors preset provisioning from a build-time generator to runtime accessors: removes the generator, CI generation workflow, and make/go:generate triggers; adds runtime chart preset functions and updates init logic to call them with error handling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer / CI
participant Repo as Repository
participant OldGen as tools/generate_presets.go
participant GenPkg as pkg/generated (vars)
participant Init as pkg/commands/init.go
participant Charts as charts/charts.go
rect rgb(240,248,255)
note right of OldGen: OLD FLOW (before)
Dev->>Repo: push
Repo->>OldGen: go generate / CI runs generator
OldGen-->>GenPkg: write PresetFiles & AvailablePresets (static)
Init->>GenPkg: read variables (compile-time)
end
rect rgb(245,255,245)
note right of Charts: NEW FLOW (after)
Dev->>Repo: push
Repo--x OldGen: generator removed
Init->>Charts: call AvailablePresets() (runtime)
Charts-->>Init: return []string, error
Init->>Charts: call PresetFiles() (runtime)
Charts-->>Init: return map[string]string, error
Init->>Init: apply presets and update TALM chart with returned data
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
![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 is a great improvement, replacing a code generation step with go:embed. This simplifies the build process and makes the project easier to maintain. The implementation is solid. I've found a couple of areas for improvement: one is a minor performance optimization in charts/charts.go by avoiding regex compilation in a loop, and the other is a more significant logic refactoring in pkg/commands/init.go to fix some redundancy and bugs in the updateTalmLibraryChart function. Overall, excellent work.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
pkg/commands/init.go (4)
130-134: Missing error handling forwriteToDestinationcall.On line 131, the error return from
writeToDestinationis discarded when writingChart.yamlfiles. This means failures are silently ignored.if parts[len(parts)-1] == "Chart.yaml" { - writeToDestination([]byte(fmt.Sprintf(content, clusterName, Config.InitOptions.Version)), file, 0o644) + err = writeToDestination([]byte(fmt.Sprintf(content, clusterName, Config.InitOptions.Version)), file, 0o644) } else { err = writeToDestination([]byte(content), file, 0o644) }
142-146: Missing error handling forwriteToDestinationcall.Same issue as above - the error return from line 143 is discarded for talm's
Chart.yaml.if parts[len(parts)-1] == "Chart.yaml" { - writeToDestination([]byte(fmt.Sprintf(content, "talm", Config.InitOptions.Version)), file, 0o644) + err = writeToDestination([]byte(fmt.Sprintf(content, "talm", Config.InitOptions.Version)), file, 0o644) } else { err = writeToDestination([]byte(content), file, 0o644) }
184-198: DuplicateRemoveAlldeletes the file just written.The code writes
talm/Chart.yamlon lines 189-193, but then immediately removes the entiretalmChartDiragain on lines 195-198. This deletes the file that was just written. The secondRemoveAllappears to be leftover from refactoring.Remove the redundant deletion:
file := filepath.Join(talmChartDir, "Chart.yaml") err = writeToDestination([]byte(fmt.Sprintf(content, "talm", Config.InitOptions.Version)), file, 0o644) if err != nil { return err } - // Remove the existing talm chart directory - if err := os.RemoveAll(talmChartDir); err != nil { - return fmt.Errorf("failed to remove existing talm chart directory: %w", err) - } - for path, content := range presetFiles {
206-210: Missing error handling forwriteToDestinationcall.Same issue - the error return from line 207 is discarded for
Chart.yaml.if parts[len(parts)-1] == "Chart.yaml" { - writeToDestination([]byte(fmt.Sprintf(content, "talm", Config.InitOptions.Version)), file, 0o644) + err = writeToDestination([]byte(fmt.Sprintf(content, "talm", Config.InitOptions.Version)), file, 0o644) } else { err = writeToDestination([]byte(content), file, 0o644) }
🧹 Nitpick comments (2)
charts/charts.go (2)
43-47: Consider compiling the regex once outside the walk function.The regex is currently compiled for each
Chart.yamlfile encountered. Moving it outside the walk function avoids repeated compilation.+var chartYamlRegex = regexp.MustCompile(`(name|version): \S+`) + // PresetFiles returns a map of file paths to their contents. // For Chart.yaml files, name and version are replaced with %s placeholders. func PresetFiles() (map[string]string, error) {Then use
chartYamlRegex.ReplaceAllString(content, "$1: %s")inside the function.
73-89: Non-deterministic ordering of presets.The order of presets (other than
genericbeing first) depends on the filesystem's directory entry order, which may vary. If consistent ordering is desired for user-facing output, consider sorting the presets.+import "sort" + // ... // Put generic first if it exists + sort.Strings(presets) if hasGeneric { presets = append([]string{"generic"}, presets...) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pkg/generated/presets.gois excluded by!**/generated/**
📒 Files selected for processing (6)
.gitignore(1 hunks)Makefile(0 hunks)charts/charts.go(1 hunks)main.go(0 hunks)pkg/commands/init.go(5 hunks)tools/generate_presets.go(0 hunks)
💤 Files with no reviewable changes (3)
- Makefile
- main.go
- tools/generate_presets.go
🧰 Additional context used
🧬 Code graph analysis (2)
charts/charts.go (1)
pkg/generated/presets.go (2)
PresetFiles(7-9)AvailablePresets(13-15)
pkg/commands/init.go (2)
charts/charts.go (2)
AvailablePresets(64-97)PresetFiles(16-60)pkg/generated/presets.go (2)
AvailablePresets(13-15)PresetFiles(7-9)
🔇 Additional comments (4)
.gitignore (1)
1-1: LGTM!The
.gitignoreentry correctly excludes thetalmdirectory from version control, aligning with the shift to embedded presets.charts/charts.go (1)
11-12: LGTM!The
all:prefix correctly ensures hidden files (like.helmignore) are included in the embedded filesystem.pkg/commands/init.go (2)
67-73: LGTM!Proper error handling for
AvailablePresets()and good refactoring to pass the presets list toisValidPresetinstead of fetching it again.
230-237: LGTM!Good refactoring to accept
availablePresetsas a parameter rather than fetching it internally.
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Signed-off-by: Andrei Kvapil kvapss@gmail.com
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.