RHTAP-6520: Render Values before Deployment#56
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR refactors the Helm values workflow by introducing ChangesValues Rendering Workflow Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 0/1 reviews remaining, refill in 34 minutes and 9 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/subcmd/deploy.go (1)
124-149: ⚡ Quick winExtract the values render/parse flow into one shared helper.
This block now duplicates the existing
SetValues/RenderValuespath ininternal/installer/installer.go, whileinternal/subcmd/template.gostill uses the older implementation. That makes the deploy and template commands easy to drift apart the next time template variables or parsing rules change. Please move this into one shared helper and have both call sites use it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/subcmd/deploy.go` around lines 124 - 149, The render/parse sequence (creating engine via NewEngine(...).Render, setting variables via variables.SetInstaller and variables.SetOpenShift, optionally printing when d.flags.Verbose, then parsing with chartutil.ReadValues) is duplicated; extract it into a single shared helper (e.g., RenderAndParseValues or RenderValuesHelper) and replace both usages so SetValues/RenderValues in installer.go and the call site in template.go and the block in deploy.go both call that helper; ensure the helper accepts the same inputs used above (context/Kube/runCtx, values template bytes, installer config/flags) and returns rendered bytes or parsed values and errors so callers (deploy.go, template.go, installer.go SetValues/RenderValues) can use it without duplicating logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/installer/installer_test.go`:
- Around line 51-64: The test mutates global os.Stdout and leaves the pipe ends
open; wrap the stdout swap and pipe close in guaranteed cleanup so state is
restored even on test failures: capture old := os.Stdout and the pipe r,w as
before, then register a t.Cleanup (or defer) that restores os.Stdout = old and
closes both w and r; ensure the call to i.PrintRawValues() happens after
replacing os.Stdout and before closing the writer, and remove any paths that
risk leaving r or w open (reference symbols: os.Stdout, os.Pipe(), old, r, w,
and i.PrintRawValues()).
---
Nitpick comments:
In `@internal/subcmd/deploy.go`:
- Around line 124-149: The render/parse sequence (creating engine via
NewEngine(...).Render, setting variables via variables.SetInstaller and
variables.SetOpenShift, optionally printing when d.flags.Verbose, then parsing
with chartutil.ReadValues) is duplicated; extract it into a single shared helper
(e.g., RenderAndParseValues or RenderValuesHelper) and replace both usages so
SetValues/RenderValues in installer.go and the call site in template.go and the
block in deploy.go both call that helper; ensure the helper accepts the same
inputs used above (context/Kube/runCtx, values template bytes, installer
config/flags) and returns rendered bytes or parsed values and errors so callers
(deploy.go, template.go, installer.go SetValues/RenderValues) can use it without
duplicating logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8e1c7ef8-d87b-4e16-bba3-c3d2422fd916
📒 Files selected for processing (3)
internal/installer/installer.gointernal/installer/installer_test.gointernal/subcmd/deploy.go
Assisted-by: Claude
3 participants
The GitOps path (
helmet-ex gitops) needs to write a singlevalues/values.yamlinto the generated repository, shared by all ArgoCD Application manifests. For this to be semantically equivalent to the imperative path, the imperative deployer must also produce values from a single render — not per-chart inside the loop.Summary by CodeRabbit
Refactor
--verboseflag with deploy, additional rendered values information is now displayed.Tests