feat: reference variables can path into related resource's variables#1005
feat: reference variables can path into related resource's variables#1005adityachoudhari26 merged 3 commits intomainfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 44 minutes and 20 seconds. ⌛ 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 (5)
📝 WalkthroughWalkthroughThe changes add support for resource-scoped variables throughout the workspace engine. A new Changes
Sequence Diagram(s)sequenceDiagram
participant VarResolver as Variable Resolver
participant PGGetter as PostgresGetter
participant DB as Database
participant ResMapper as Resource Mapper
participant PropGetter as Property Getter
VarResolver->>PGGetter: LoadCandidates(resource)
PGGetter->>DB: ListResourceVariablesByWorkspaceID
DB-->>PGGetter: resource variables rows
PGGetter->>PGGetter: Convert to oapi.Value map
PGGetter-->>VarResolver: EntityData with variables in Raw
VarResolver->>ResMapper: mapToResource(entityData)
ResMapper->>ResMapper: Extract variables from Raw
ResMapper-->>VarResolver: oapi.Resource with Variables populated
VarResolver->>PropGetter: getResourceProperty(resource, "variables.db_url")
PropGetter->>PropGetter: getResourceVariableProperty
PropGetter->>PropGetter: Lookup variable, validate, convert
PropGetter-->>VarResolver: Resolved variable value
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/workspace-engine/test/controllers/variable_test.go (1)
523-553: Nit: missing section-header comment for consistency.Every other test in this file (including the sibling test added right below at line 559) is preceded by a
// ----style section-header comment describing the case. Please add one here too so the file style stays uniform.✏️ Proposed addition
+// --------------------------------------------------------------------------- +// Reference variable resolves from a variable on a related resource +// --------------------------------------------------------------------------- + func TestVariable_ReferenceVariable_ResolvesFromRelatedResourceVariables(t *testing.T) {As per coding guidelines: *"Follow the existing test structure used in _test.go files".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/test/controllers/variable_test.go` around lines 523 - 553, Add the missing section-header comment above the test function TestVariable_ReferenceVariable_ResolvesFromRelatedResourceVariables to match the file's style (a `// ---`-style header describing the case), using the same short descriptive phrasing pattern as the sibling test below (e.g., a one-line `// --- Resolve reference variable from related resource variables` comment) so the test file remains consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/workspace-engine/test/controllers/variable_test.go`:
- Around line 523-553: Add the missing section-header comment above the test
function TestVariable_ReferenceVariable_ResolvesFromRelatedResourceVariables to
match the file's style (a `// ---`-style header describing the case), using the
same short descriptive phrasing pattern as the sibling test below (e.g., a
one-line `// --- Resolve reference variable from related resource variables`
comment) so the test file remains consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a0ba2bd8-1d51-48bc-a940-ab9bd74ed8e9
📒 Files selected for processing (8)
apps/workspace-engine/oapi/openapi.jsonapps/workspace-engine/oapi/spec/schemas/entities.jsonnetapps/workspace-engine/pkg/oapi/oapi.gen.goapps/workspace-engine/pkg/workspace/relationships/property.goapps/workspace-engine/svc/controllers/desiredrelease/variableresolver/getters_postgres.goapps/workspace-engine/svc/controllers/desiredrelease/variableresolver/resolve.goapps/workspace-engine/test/controllers/harness/pipeline_opts.goapps/workspace-engine/test/controllers/variable_test.go
There was a problem hiding this comment.
Pull request overview
Adds support for reference variables to traverse into a related resource’s variables (not just config/metadata), addressing issue #890 in the workspace-engine variable resolution flow.
Changes:
- Extend relationship property traversal to support
resource.variables.<key>[.<nested>...]. - Ensure related resource candidates (test harness + Postgres getter) include resource variables in their raw entity maps for reference resolution.
- Update the workspace-engine OpenAPI Resource schema/type to include an optional
variablesmap.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/workspace-engine/test/controllers/variable_test.go | Adds controller-level tests covering reference resolution from related resource variables (including nested object traversal). |
| apps/workspace-engine/test/controllers/harness/pipeline_opts.go | Includes related resource variables in scenario candidate Raw maps to enable reference traversal in tests. |
| apps/workspace-engine/svc/controllers/desiredrelease/variableresolver/resolve.go | Maps variables from EntityData.Raw into oapi.Resource so relationship property lookup can access them. |
| apps/workspace-engine/svc/controllers/desiredrelease/variableresolver/getters_postgres.go | Loads resource variables from Postgres and attaches them to resource candidate Raw maps and GetEntityByID results. |
| apps/workspace-engine/pkg/workspace/relationships/property.go | Adds variables handling for resource property traversal, including nested object lookups. |
| apps/workspace-engine/pkg/oapi/oapi.gen.go | Updates generated oapi.Resource type to include optional Variables. |
| apps/workspace-engine/oapi/spec/schemas/entities.jsonnet | Adds variables to the Resource schema in the OAPI spec source. |
| apps/workspace-engine/oapi/openapi.json | Updates the generated OpenAPI artifact to include the Resource variables field. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| rows, err := q.ListActiveResourcesByWorkspace(ctx, workspaceID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("list resources for workspace %s: %w", workspaceID, err) | ||
| } | ||
| varsByResource, err := loadResourceVariablesByWorkspace(ctx, q, workspaceID) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| candidates := make([]eval.EntityData, 0, len(rows)) | ||
| for _, r := range rows { | ||
| candidates = append(candidates, eval.EntityData{ | ||
| ID: r.ID, | ||
| WorkspaceID: r.WorkspaceID, | ||
| EntityType: "resource", | ||
| Raw: resourceRowToMap(r), | ||
| Raw: resourceRowToMap(r, varsByResource[r.ID]), | ||
| }) |
There was a problem hiding this comment.
LoadCandidates now fetches all resource variables for the workspace in a separate query. The underlying ListResourceVariablesByWorkspaceID SQL only filters by workspace_id (no deleted_at IS NULL), so this will also pull variables for soft-deleted resources and any other inactive rows, increasing work/memory for large workspaces. Consider tightening the query to active resources (e.g., join with resource.deleted_at IS NULL) or otherwise scoping the variable load to the active resource IDs returned by ListActiveResourcesByWorkspace.
| varsByResource, err := loadResourceVariablesByWorkspace(ctx, q, workspaceID) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| candidates := make([]eval.EntityData, 0, len(rows)) | ||
| for _, r := range rows { | ||
| candidates = append(candidates, eval.EntityData{ | ||
| ID: r.ID, | ||
| WorkspaceID: r.WorkspaceID, | ||
| EntityType: "resource", | ||
| Raw: resourceRowToMap(r), | ||
| Raw: resourceRowToMap(r, varsByResource[r.ID]), | ||
| }) |
There was a problem hiding this comment.
The new behavior of loading and attaching variables into resource candidates (via loadResourceVariablesByWorkspace/resourceRowToMap) isn’t covered by the existing PostgresGetter tests. Adding a test that inserts rows into resource_variable and asserts LoadCandidates includes a variables entry in EntityData.Raw would help prevent regressions and ensure DB-backed reference-path resolution works.
| case "variables": | ||
| return getResourceVariableProperty(resource, propertyPath) | ||
| default: | ||
| return getPropertyReflection(resource, propertyPath) | ||
| } |
There was a problem hiding this comment.
New variables path handling for resources (getResourceVariableProperty) isn’t covered by the existing property extraction unit tests. Adding tests for variables/<key> and nested object traversal (plus key-missing / non-literal cases) would help prevent regressions.
|
|
||
| obj, err := lv.AsObjectValue() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("cannot traverse into non-object variable %s", propertyPath[1]) |
There was a problem hiding this comment.
When AsObjectValue() fails, the returned error drops the underlying reason (err). Wrapping the original error (e.g., with %w) would make it much easier to debug why traversal failed (wrong type vs malformed literal, etc.).
| return nil, fmt.Errorf("cannot traverse into non-object variable %s", propertyPath[1]) | |
| return nil, fmt.Errorf("cannot traverse into non-object variable %s: %w", propertyPath[1], err) |
fixes #890
Summary by CodeRabbit
New Features
Tests