Standardize Go templating variable casing to lowercase #739
Conversation
This change addresses issue #738 by standardizing all Go templating variables to use lowercase consistently across all templating surfaces. Changes: - Added Map() method to TemplatableJob that converts struct fields to a map with lowercase keys (using JSON tag names) - Updated ArgoCD dispatcher to use Map() for template execution - Updated TerraformCloud dispatcher to use Map() for template execution - Updated all tests to use lowercase template variables - Updated ArgoCD frontend default config example to use lowercase Template variables now consistently use: - {{.resource.name}} instead of {{.Resource.Name}} - {{.deployment.slug}} instead of {{.Deployment.Slug}} - {{.environment.name}} instead of {{.Environment.Name}} - {{.release.version.tag}} instead of {{.Release.Version.Tag}} - {{.release.variables}} instead of {{.Release.Variables}} - {{.job.id}} instead of {{.Job.Id}} This aligns job dispatch templating with verification templating, which already used lowercase variables via ProviderContext.Map(). Closes #738 Co-authored-by: justin <justin@wandb.com>
|
Cursor Agent can help with this pull request. Just |
📝 WalkthroughWalkthroughThis PR standardizes template variable names to lowercase across the templating system. It introduces a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
|
closes #738 |
jsbroks
changed the title
ctrlplane.yaml support![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on January 28
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| releaseMap := structToMap(t.Release.Release) | ||
| releaseMap["variables"] = t.Release.Variables | ||
| result["release"] = releaseMap | ||
| } |
There was a problem hiding this comment.
Nil map assignment causes panic in Map()
Medium Severity
The structToMap function returns nil on JSON marshal or unmarshal errors (lines 93-98). When processing the release in Map(), line 81 calls structToMap(t.Release.Release) and line 82 immediately assigns to the result with releaseMap["variables"] = t.Release.Variables. If structToMap returns nil, this assignment to a nil map causes a panic. Unlike the other field assignments (resource, deployment, environment), the release handling requires the returned map to be non-nil for the subsequent assignment.
![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
🤖 Fix all issues with AI agents
In `@apps/workspace-engine/pkg/oapi/job.go`:
- Around line 80-83: The code writes releaseMap["variables"] without guarding
against structToMap returning nil; update the block handling t.Release so after
calling structToMap(t.Release.Release) you check if releaseMap == nil and if so
initialize it to an empty map[string]interface{} (or otherwise ensure it's
non-nil) before assigning releaseMap["variables"] and setting result["release"];
reference the t.Release, structToMap, releaseMap and result["release"]
identifiers when making the change.
🧹 Nitpick comments (1)
apps/workspace-engine/pkg/oapi/job.go (1)
58-78: Remove redundant inline comments inMap()These inline comments mostly restate the code and add noise. Suggest trimming them to keep the function lean. As per coding guidelines, avoid obvious inline comments in
apps/workspace-engine/**/*.go.♻️ Proposed cleanup
- // Convert each field to a map using JSON marshal/unmarshal - // This ensures all keys are lowercase per JSON tags - - // Resource if t.Resource != nil { result["resource"] = structToMap(t.Resource) } - // Deployment if t.Deployment != nil { result["deployment"] = structToMap(t.Deployment) } - // Environment if t.Environment != nil { result["environment"] = structToMap(t.Environment) } - // Job result["job"] = structToMap(t.Job) - // Release with variables if t.Release != nil {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
apps/web/app/routes/ws/deployments/settings/_components/ArgoCD.tsxapps/workspace-engine/pkg/oapi/job.goapps/workspace-engine/pkg/oapi/job_test.goapps/workspace-engine/pkg/workspace/jobdispatch/argocd.goapps/workspace-engine/pkg/workspace/jobdispatch/argocd_test.goapps/workspace-engine/pkg/workspace/jobdispatch/terraformcloud.go
🧰 Additional context used
📓 Path-based instructions (4)
apps/workspace-engine/**/*.go
📄 CodeRabbit inference engine (apps/workspace-engine/CLAUDE.md)
apps/workspace-engine/**/*.go: Do not add extraneous inline comments that state the obvious
Do not add comments that simply restate what the code does
Do not add comments for standard Go patterns (e.g., noting WaitGroup or semaphore usage)
Write comments that explain why, document complex logic/algorithms, provide non-obvious context, include TODO/FIXME, and document exported functions/types/methods
Files:
apps/workspace-engine/pkg/workspace/jobdispatch/terraformcloud.goapps/workspace-engine/pkg/workspace/jobdispatch/argocd.goapps/workspace-engine/pkg/oapi/job.goapps/workspace-engine/pkg/oapi/job_test.goapps/workspace-engine/pkg/workspace/jobdispatch/argocd_test.go
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript with explicit types (prefer interfaces for public APIs)
Import styles: Use named imports, group imports by source (std lib > external > internal)
Consistent type imports:import type { Type } from "module"
Prefer async/await over raw promises
Handle errors explicitly (use try/catch and typed error responses)
Files:
apps/web/app/routes/ws/deployments/settings/_components/ArgoCD.tsx
⚙️ CodeRabbit configuration file
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
Files:
apps/web/app/routes/ws/deployments/settings/_components/ArgoCD.tsx
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (CLAUDE.md)
Formatting: Prettier is used with
@ctrlplane/prettier-config
Files:
apps/web/app/routes/ws/deployments/settings/_components/ArgoCD.tsx
apps/workspace-engine/**/*_test.go
📄 CodeRabbit inference engine (apps/workspace-engine/CLAUDE.md)
Follow the existing test structure used in *_test.go files
Files:
apps/workspace-engine/pkg/oapi/job_test.goapps/workspace-engine/pkg/workspace/jobdispatch/argocd_test.go
🧠 Learnings (6)
📚 Learning: 2025-08-12T18:13:54.630Z
Learnt from: CR
Repo: ctrlplanedev/ctrlplane PR: 0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/pkg/mapping/protobuf_mappings.go : Add mapping functions for the new condition in pkg/mapping/protobuf_mappings.go
Applied to files:
apps/workspace-engine/pkg/oapi/job.goapps/workspace-engine/pkg/oapi/job_test.go
📚 Learning: 2025-08-12T18:13:54.630Z
Learnt from: CR
Repo: ctrlplanedev/ctrlplane PR: 0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/**/*_test.go : Follow the existing test structure used in *_test.go files
Applied to files:
apps/workspace-engine/pkg/oapi/job_test.goapps/workspace-engine/pkg/workspace/jobdispatch/argocd_test.go
📚 Learning: 2025-08-12T18:13:54.630Z
Learnt from: CR
Repo: ctrlplanedev/ctrlplane PR: 0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/pkg/model/selector/**/*_test.go : Use table-driven tests for all condition types
Applied to files:
apps/workspace-engine/pkg/oapi/job_test.goapps/workspace-engine/pkg/workspace/jobdispatch/argocd_test.go
📚 Learning: 2025-08-12T18:13:54.630Z
Learnt from: CR
Repo: ctrlplanedev/ctrlplane PR: 0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/pkg/model/selector/**/*_test.go : Write comprehensive, data-driven tests for new condition types
Applied to files:
apps/workspace-engine/pkg/oapi/job_test.go
📚 Learning: 2025-08-12T18:13:54.630Z
Learnt from: CR
Repo: ctrlplanedev/ctrlplane PR: 0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/pkg/model/selector/**/*_test.go : Test validation and matching logic separately for condition types
Applied to files:
apps/workspace-engine/pkg/oapi/job_test.go
📚 Learning: 2025-08-12T18:13:54.630Z
Learnt from: CR
Repo: ctrlplanedev/ctrlplane PR: 0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/pkg/model/selector/**/*_test.go : Include edge cases in tests (empty values, special characters, unicode) for condition types
Applied to files:
apps/workspace-engine/pkg/oapi/job_test.goapps/workspace-engine/pkg/workspace/jobdispatch/argocd_test.go
🧬 Code graph analysis (1)
apps/workspace-engine/pkg/oapi/job.go (2)
apps/workspace-engine/pkg/oapi/oapi.gen.go (1)
Release(902-908)apps/workspace-engine/pkg/workspace/store/variables.go (1)
Variables(11-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Cursor Bugbot
- GitHub Check: build (linux/amd64)
- GitHub Check: tests
- GitHub Check: tests
🔇 Additional comments (13)
apps/workspace-engine/pkg/workspace/jobdispatch/terraformcloud.go (1)
200-202: Map()-based template input looks correctPassing
job.Map()here matches the new lowercase template conventions and keeps rendering consistent.apps/workspace-engine/pkg/workspace/jobdispatch/argocd.go (1)
200-202: Template execution now aligned with lowercase mapSwitching to
templatableJobWithRelease.Map()matches the standardized key casing.apps/web/app/routes/ws/deployments/settings/_components/ArgoCD.tsx (1)
18-38: Default ArgoCD YAML placeholders now match lowercase keysThese updates line up with the new templating data shape and should reduce user confusion.
apps/workspace-engine/pkg/oapi/job_test.go (1)
11-169: Test coverage for Map()/structToMap is solidNice coverage of nested structures and nil-field cases; this should help prevent regressions in templating.
apps/workspace-engine/pkg/workspace/jobdispatch/argocd_test.go (9)
299-309: Map-based execution matches dispatcher behavior.Nice alignment with the runtime template input.
385-431: Lowercase placeholders in basic field tests look consistent.
468-533: Resource config + sprig helper templates updated cleanly.
536-575: Full ArgoCD template case alignment looks solid.
577-586: Nil-resource guard remains effective with lowercase access.
588-611: Invalid-template coverage remains intact after casing update.
788-803: Dispatcher template strings updated consistently to lowercase.Also applies to: 863-870
970-1004: Dispatcher invalid-template paths still well covered.Also applies to: 1188-1231
434-456: The test expectation is correct.TemplatableRelease.Variablesismap[string]string(notmap[string]any), andMap()assigns it directly without conversion at line 84 ofjob.go. When accessing a missing key viaindexon amap[string]stringin Go templates, the result is an empty string, which matches the test's expected behavior. No changes needed.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| if t.Release != nil { | ||
| releaseMap := structToMap(t.Release.Release) | ||
| releaseMap["variables"] = t.Release.Variables | ||
| result["release"] = releaseMap |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate the file and check its content
fd job.go -p apps/workspace-engine/pkg/oapiRepository: ctrlplanedev/ctrlplane
Length of output: 103
🏁 Script executed:
# Read the job.go file around the mentioned lines
head -n 110 apps/workspace-engine/pkg/oapi/job.go | tail -n 40Repository: ctrlplanedev/ctrlplane
Length of output: 931
🏁 Script executed:
# Find the structToMap function definition
rg -n "func structToMap" apps/workspace-engineRepository: ctrlplanedev/ctrlplane
Length of output: 253
Prevent nil-map panic when mapping release data
structToMap returns nil on marshal/unmarshal failure, but releaseMap["variables"] is written unconditionally at line 82. This will panic if the JSON conversion fails. Add a nil guard before accessing the map.
Proposed fix
if t.Release != nil {
releaseMap := structToMap(t.Release.Release)
- releaseMap["variables"] = t.Release.Variables
- result["release"] = releaseMap
+ if releaseMap != nil {
+ releaseMap["variables"] = t.Release.Variables
+ result["release"] = releaseMap
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if t.Release != nil { | |
| releaseMap := structToMap(t.Release.Release) | |
| releaseMap["variables"] = t.Release.Variables | |
| result["release"] = releaseMap | |
| if t.Release != nil { | |
| releaseMap := structToMap(t.Release.Release) | |
| if releaseMap != nil { | |
| releaseMap["variables"] = t.Release.Variables | |
| result["release"] = releaseMap | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@apps/workspace-engine/pkg/oapi/job.go` around lines 80 - 83, The code writes
releaseMap["variables"] without guarding against structToMap returning nil;
update the block handling t.Release so after calling
structToMap(t.Release.Release) you check if releaseMap == nil and if so
initialize it to an empty map[string]interface{} (or otherwise ensure it's
non-nil) before assigning releaseMap["variables"] and setting result["release"];
reference the t.Release, structToMap, releaseMap and result["release"]
identifiers when making the change.
Standardize Go templating variable casing to lowercase for consistency across all templating surfaces.
Previously, verification templating used lowercase variables (via JSON serialization), while job dispatching used PascalCase directly from Go structs, leading to inconsistent template variable access. This change aligns job dispatching with the lowercase convention.
Note
Unifies template variable casing to lowercase across job dispatchers and UI.
TemplatableJob.Map()(withstructToMap) to expose lowercase, JSON-tagged fields for templating (e.g.,{{.resource.name}},{{.release.variables}})Map()instead of struct fieldsoapitests forMap()behavior; ArgoCD dispatcher tests and templates migrated to lowercase variable accessWritten by Cursor Bugbot for commit aee5bbc. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.