chore: init resource variable repository #661
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a Variable interface, a ResourceVariable abstraction and DirectResourceVariable implementation, an in-memory thread-safe ResourceVariableRepository with CRUD/uniqueness semantics, and comprehensive repository tests covering normal flows and error cases. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Var as DirectResourceVariable
Caller->>Var: ResolveValue(ctx, releaseTarget)
alt v.IsSensitive() == true
Var-->>Caller: error("sensitive variable not supported")
else
Note over Var: return fmt.Sprintf("%v", value)
Var-->>Caller: string value
end
sequenceDiagram
autonumber
actor Service
participant Repo as ResourceVariableRepository
participant Map as variables[resourceID][key]
rect rgba(200,240,255,0.12)
note over Service,Repo: Create
Service->>Repo: Create(ctx, variable)
Repo->>Repo: validate non-nil, ensure bucket exists
Repo->>Map: check key uniqueness and global ID uniqueness
alt unique
Repo->>Map: store copy
Repo-->>Service: ok
else duplicate
Repo-->>Service: error (duplicate key or id)
end
end
rect rgba(220,255,220,0.12)
note over Service,Repo: Update
Service->>Repo: Update(ctx, variable)
Repo->>Map: ensure bucket and key exist
Repo->>Repo: verify provided ID matches existing entry
alt valid
Repo->>Map: replace with copy
Repo-->>Service: ok
else invalid
Repo-->>Service: error
end
end
rect rgba(255,230,200,0.12)
note over Service,Repo: Delete
Service->>Repo: Delete(ctx, id)
Repo->>Map: scan buckets, remove entry with matching id
Repo-->>Service: ok
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (11)
apps/workspace-engine/pkg/engine/variable/variable.go (1)
8-12: Add GoDoc for the exported Variable interfaceExported identifiers should have comments for godoc and linting. Briefly describe the contract, especially the ResolveValue semantics and how ReleaseTarget is intended to influence resolution.
type Variable interface { - GetID() string - GetKey() string - ResolveValue(ctx context.Context, releaseTarget *rt.ReleaseTarget) (string, error) + // GetID returns a stable, unique identifier for the variable. + GetID() string + // GetKey returns the human-readable key/name of the variable within its scope. + GetKey() string + // ResolveValue returns the variable's value as a string for the given release target. + // Implementations may consult releaseTarget to compute a context-aware value. + ResolveValue(ctx context.Context, releaseTarget *rt.ReleaseTarget) (string, error) }apps/workspace-engine/pkg/engine/variable/resource_variable.go (4)
10-13: Add GoDoc on ResourceVariable to clarify scope and identity guaranteesDocument that ResourceVariable is scoped by ResourceID and keyed by GetKey, and clarify expected uniqueness (per resource/key vs global ID uniqueness).
-type ResourceVariable interface { +// ResourceVariable represents a variable scoped to a specific resource. +// Implementations must be uniquely identifiable per (resourceID, key) pair. +type ResourceVariable interface { GetResourceID() string Variable }
15-21: Add GoDoc on DirectResourceVariable and its mutability expectationsExplain that fields are intentionally unexported and whether instances are expected to be immutable after creation (relevant for repository copy semantics).
-type DirectResourceVariable struct { +// DirectResourceVariable is a simple in-memory implementation of ResourceVariable. +// Note: values are mutable via exported getters; repository code should avoid aliasing +// if immutability is desired. +type DirectResourceVariable struct { id string resourceID string key string value any sensitive bool }
3-9: Introduce a sentinel error for sensitive variables; avoid stringly-typed error checksExport a package-level var so callers can reliably check errors with errors.Is. Also reuse it in tests, if desired, later.
import ( "context" "errors" "fmt" rt "workspace-engine/pkg/engine/policy/releasetargets" ) +// ErrSensitiveVariableUnsupported is returned when a sensitive variable is asked to resolve its raw value. +var ErrSensitiveVariableUnsupported = errors.New("sensitive variable not supported") + func (v *DirectResourceVariable) ResolveValue(_ context.Context, __ *rt.ReleaseTarget) (string, error) { if v.sensitive { // TODO: encryption - return "", errors.New("sensitive variable not supported") + return "", ErrSensitiveVariableUnsupported } return fmt.Sprintf("%v", v.value), nil }Also applies to: 43-50
35-37: Consider constraining GetValue/ResolveValue types to reduce surprisesReturning any + fmt.Sprintf("%v", v.value) can lead to surprising string encodings (e.g., maps, structs) and accidental leakage. If only strings are expected downstream, prefer storing string (or a small set of supported types) and validating at construction.
Also applies to: 49-49
apps/workspace-engine/pkg/engine/variable/resource_variable_repository.go (2)
13-20: Avoid pointer-to-interface throughout; it complicates nil handling and copy semanticsUsing *ResourceVariable (pointer to interface) is an anti-pattern that leads to:
- Double indirection and tricky nil checks (variable == nil || *variable == nil).
- Accidental aliasing: copying an interface that contains a pointer (e.g., *DirectResourceVariable) does not deep-copy the underlying struct; changes can leak into the repository.
Consider storing ResourceVariable (interface) in the map and returning copies by value or building a dedicated DTO. Method signatures must conform to model.Repository[T], but internal storage can be simplified:- type ResourceVariableRepository struct { - variables map[string]map[string]*ResourceVariable // resourceID -> key -> variable + type ResourceVariableRepository struct { + variables map[string]map[string]ResourceVariable // resourceID -> key -> variable mu sync.RWMutex } - func NewResourceVariableRepository() *ResourceVariableRepository { - return &ResourceVariableRepository{variables: make(map[string]map[string]*ResourceVariable)} + func NewResourceVariableRepository() *ResourceVariableRepository { + return &ResourceVariableRepository{variables: make(map[string]map[string]ResourceVariable)} }Internally, Create/Update would assign the interface value (not its address); getters would allocate a fresh local and return its address to satisfy []*T return types. If stronger immutability is required, introduce a Clone() ResourceVariable on the interface and clone on write.
Also applies to: 22-23, 60-61, 79-81, 96-104, 121-129
178-189: Exists is O(N) scan; consider keeping an ID index for O(1) lookupsIf this repository is used on hot paths, add a secondary index map[id]struct{} or map[id]ref to accelerate Get/Exists/Delete operations.
apps/workspace-engine/pkg/engine/variable/resource_variable_repository_test.go (4)
32-39: Mark helpers with t.Helper() for accurate failure reportingAdd t.Helper() at the start of helper methods so line numbers point to the caller on failures.
func (b *testBundle) assertEqualError(actualError error) { + b.t.Helper() if b.step.expectedError == nil { assert.NilError(b.t, actualError) return } assert.Error(b.t, actualError, b.step.expectedError.Error()) }Apply similarly in assertVariableEqual, validateVariableReturnedFromGet, validateVariableReturnedFromGetAll, validateVariableReturnedFromGetAllByResourceID, validateVariableReturnedFromGetByResourceIDAndKey, validateVariableExists, and validateVariableState.
80-93: validateVariableReturnedFromGetAll returns after first element; it can miss validating the intended variableMatch by ID (or resourceID+key) instead of returning on the first entry. This also helps catch aliasing bugs in GetAll.
func (b *testBundle) validateVariableReturnedFromGetAll(variable *DirectResourceVariable) *testBundle { allVariables := b.repo.GetAll(b.ctx) - for _, actualVariable := range allVariables { - if actualVariable == nil || *actualVariable == nil { - continue - } - - b.assertVariableEqual(*actualVariable, variable) - return b - } - - b.t.Errorf("variable not found in GetAll") + for _, actualVariable := range allVariables { + if actualVariable == nil || *actualVariable == nil { + continue + } + if (*actualVariable).GetID() == variable.GetID() { + b.assertVariableEqual(*actualVariable, variable) + return b + } + } + b.t.Errorf("variable with ID %s not found in GetAll", variable.GetID()) return b }
439-458: Nil equality assertion can be simplifiedComparing to a typed nil pointer is unnecessary; nonExistent == nil is sufficient and clearer.
- nonExistent := repo.Get(ctx, "non-existent") - assert.Assert(t, nonExistent == (*ResourceVariable)(nil)) + nonExistent := repo.Get(ctx, "non-existent") + assert.Assert(t, nonExistent == nil)
146-171: Consider adding a multi-variable scenario to expose the GetAll address-of-loop bugCreate two variables under the same resource (distinct keys) and assert both are returned correctly from GetAll and GetAllByResourceID with distinct IDs/values.
I can draft this additional test block if you want to include it now.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
apps/workspace-engine/pkg/engine/variable/resource_variable.go(1 hunks)apps/workspace-engine/pkg/engine/variable/resource_variable_repository.go(1 hunks)apps/workspace-engine/pkg/engine/variable/resource_variable_repository_test.go(1 hunks)apps/workspace-engine/pkg/engine/variable/variable.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
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/engine/variable/variable.goapps/workspace-engine/pkg/engine/variable/resource_variable.goapps/workspace-engine/pkg/engine/variable/resource_variable_repository_test.goapps/workspace-engine/pkg/engine/variable/resource_variable_repository.go
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/engine/variable/resource_variable_repository_test.go
🧠 Learnings (1)
📚 Learning: 2025-08-12T18:13:54.630Z
Learnt from: CR
PR: ctrlplanedev/ctrlplane#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/engine/variable/resource_variable_repository_test.go
🧬 Code graph analysis (3)
apps/workspace-engine/pkg/engine/variable/resource_variable.go (1)
apps/workspace-engine/pkg/engine/variable/variable.go (1)
Variable(8-12)
apps/workspace-engine/pkg/engine/variable/resource_variable_repository_test.go (2)
apps/workspace-engine/pkg/engine/variable/resource_variable.go (2)
DirectResourceVariable(15-21)ResourceVariable(10-13)apps/workspace-engine/pkg/engine/variable/resource_variable_repository.go (2)
ResourceVariableRepository(13-16)NewResourceVariableRepository(18-20)
apps/workspace-engine/pkg/engine/variable/resource_variable_repository.go (2)
apps/workspace-engine/pkg/model/repository.go (1)
Repository(7-14)apps/workspace-engine/pkg/engine/variable/resource_variable.go (1)
ResourceVariable(10-13)
⏰ 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). (2)
- GitHub Check: Typecheck
- GitHub Check: Lint
🔇 Additional comments (1)
apps/workspace-engine/pkg/engine/variable/resource_variable_repository.go (1)
26-36: Bug: taking address of a loop-scoped value returns duplicate pointers (all entries alias the same memory)Each append takes &variableCopy; variableCopy is reused per iteration, so all returned pointers can point to the last value. Allocate a new variable per iteration before taking the address.
- var variables []*ResourceVariable - for _, resourceVariables := range r.variables { - for _, variable := range resourceVariables { - variableCopy := *variable - if variableCopy == nil { - continue - } - - variables = append(variables, &variableCopy) - } - } + var variables []*ResourceVariable + for _, resourceVariables := range r.variables { + for _, variable := range resourceVariables { + if variable == nil || *variable == nil { + continue + } + vc := *variable // copy interface value + variables = append(variables, &vc) + } + }Likely an incorrect or invalid review comment.
| deleteResourceVariable := resourceVariableRepositoryTest{ | ||
| name: "deletes a resource variable", | ||
| steps: []resourceVariableRepositoryTestStep{ | ||
| { | ||
| createResourceVariable: &DirectResourceVariable{ | ||
| id: "1", | ||
| resourceID: "1", | ||
| key: "key", | ||
| value: "value", | ||
| sensitive: false, | ||
| }, | ||
| expectedResourceVariables: map[string]map[string]*DirectResourceVariable{ | ||
| "1": { | ||
| "key": { | ||
| id: "1", | ||
| resourceID: "1", | ||
| key: "key", | ||
| value: "value", | ||
| sensitive: false, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| deleteResourceVariable: &DirectResourceVariable{id: "1"}, | ||
| expectedResourceVariables: map[string]map[string]*DirectResourceVariable{"1": {}}, | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add a test to catch Delete bug when variable ID differs from resourceID
Current Delete only searches r.variables[id]; it fails when id != resourceID. Add a case where they differ to ensure deletion works.
@@
deleteResourceVariable := resourceVariableRepositoryTest{
name: "deletes a resource variable",
steps: []resourceVariableRepositoryTestStep{
{
- createResourceVariable: &DirectResourceVariable{
- id: "1",
- resourceID: "1",
+ createResourceVariable: &DirectResourceVariable{
+ id: "var-1",
+ resourceID: "res-1",
key: "key",
value: "value",
sensitive: false,
},
expectedResourceVariables: map[string]map[string]*DirectResourceVariable{
- "1": {
+ "res-1": {
"key": {
- id: "1",
- resourceID: "1",
+ id: "var-1",
+ resourceID: "res-1",
key: "key",
value: "value",
sensitive: false,
},
},
},
},
{
- deleteResourceVariable: &DirectResourceVariable{id: "1"},
- expectedResourceVariables: map[string]map[string]*DirectResourceVariable{"1": {}},
+ deleteResourceVariable: &DirectResourceVariable{id: "var-1"},
+ expectedResourceVariables: map[string]map[string]*DirectResourceVariable{"res-1": {}},
},
},
}This should fail on current implementation and pass with the proposed Delete fix.
🏁 Script executed:
#!/bin/bash
# Locate the resource_variable_repository.go implementation file
find . -type f -name 'resource_variable_repository.go'Length of output: 130
🏁 Script executed:
# Display the struct definition and Delete method implementation for verification
sed -n '1,200p' apps/workspace-engine/pkg/engine/variable/resource_variable_repository.go | sed -n '1,150p'Length of output: 3639
🏁 Script executed:
# Display the Delete method implementation for verification
sed -n '150,300p' apps/workspace-engine/pkg/engine/variable/resource_variable_repository.goLength of output: 931
Fix Delete to locate variable by ID across all resources and add corresponding test
The current Delete implementation treats the input id as a resourceID, so it only looks in r.variables[id] and no‐ops when the variable’s ID differs from its resource’s ID. To address this:
- Location to update Delete logic
apps/workspace-engine/pkg/engine/variable/resource_variable_repository.go: Delete(ctx context.Context, id string) - Location to add test
apps/workspace-engine/pkg/engine/variable/resource_variable_repository_test.goaround lines 219–247
Proposed changes:
--- a/apps/workspace-engine/pkg/engine/variable/resource_variable_repository.go
+++ b/apps/workspace-engine/pkg/engine/variable/resource_variable_repository.go
@@ func (r *ResourceVariableRepository) Delete(ctx context.Context, id string) error {
- if resourceVariables, ok := r.variables[id]; ok {
- for _, variable := range resourceVariables {
- variableCopy := *variable
- if variableCopy == nil {
- continue
- }
- if variableCopy.GetID() == id {
- delete(resourceVariables, variableCopy.GetKey())
- }
- }
- }
+ // Iterate all resources to find the variable matching the given ID.
+ for resourceID, resourceVariables := range r.variables {
+ for key, variable := range resourceVariables {
+ if variable == nil {
+ continue
+ }
+ if variable.GetID() == id {
+ delete(r.variables[resourceID], key)
+ // Clean up empty resource entry if desired
+ if len(r.variables[resourceID]) == 0 {
+ delete(r.variables, resourceID)
+ }
+ return nil
+ }
+ }
+ }
return nil
}--- a/apps/workspace-engine/pkg/engine/variable/resource_variable_repository_test.go
+++ b/apps/workspace-engine/pkg/engine/variable/resource_variable_repository_test.go
@@ steps: []resourceVariableRepositoryTestStep{
{
- createResourceVariable: &DirectResourceVariable{
- id: "1",
- resourceID: "1",
+ createResourceVariable: &DirectResourceVariable{
+ id: "var-1",
+ resourceID: "res-1",
key: "key",
value: "value",
sensitive: false,
@@
- deleteResourceVariable: &DirectResourceVariable{id: "1"},
- expectedResourceVariables: map[string]map[string]*DirectResourceVariable{"1": {}},
+ deleteResourceVariable: &DirectResourceVariable{id: "var-1"},
+ expectedResourceVariables: map[string]map[string]*DirectResourceVariable{"res-1": {}},With these updates, the test will catch the bug when a variable’s ID differs from its resourceID and verify that deletion succeeds.
Committable suggestion skipped: line range outside the PR's diff.
apps/workspace-engine/pkg/engine/variable/resource_variable_repository.go
Show resolved
Hide resolved
apps/workspace-engine/pkg/engine/variable/resource_variable_repository.go
Show resolved
Hide resolved
apps/workspace-engine/pkg/engine/variable/resource_variable_repository.go
Show resolved
Hide resolved
apps/workspace-engine/pkg/engine/variable/resource_variable_repository.go
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/workspace-engine/pkg/engine/variable/resource_variable_repository.go (1)
158-170: Delete should remove by map key, stop after success, and optionally prune empty bucketsUsing
variableCopy.GetKey()to delete can fail if the key on the value mutated post-insert. Also, return immediately after deletion and optionally clean up empty resource buckets.Apply this diff:
- for _, resourceVariables := range r.variables { - for _, variable := range resourceVariables { - if variable == nil || *variable == nil { - continue - } - variableCopy := *variable - if variableCopy.GetID() == id { - delete(resourceVariables, variableCopy.GetKey()) - } - } - } + for resourceID, resourceVariables := range r.variables { + for key, variable := range resourceVariables { + if variable == nil || *variable == nil { + continue + } + if (*variable).GetID() == id { + delete(resourceVariables, key) + if len(resourceVariables) == 0 { + delete(r.variables, resourceID) + } + return nil + } + } + } return nil
🧹 Nitpick comments (3)
apps/workspace-engine/pkg/engine/variable/resource_variable_repository.go (3)
132-147: Make Update errors more actionable with identifiersThese error messages are a bit opaque during debugging. Include the resourceID/key and IDs to aid diagnostics.
Apply this diff:
- if _, ok := r.variables[resourceID]; !ok { - return errors.New("resource not found") - } + if _, ok := r.variables[resourceID]; !ok { + return fmt.Errorf("resource %q not found", resourceID) + } current, ok := r.variables[resourceID][key] if !ok { - return errors.New("variable key not found") + return fmt.Errorf("variable key %q not found for resource %q", key, resourceID) } currentCopy := *current if currentCopy == nil { return errors.New("current variable is nil") } if currentCopy.GetID() != variableCopy.GetID() { - return errors.New("variable ID mismatch") + return fmt.Errorf("variable ID mismatch: existing=%q provided=%q", currentCopy.GetID(), variableCopy.GetID()) }
13-20: Add doc comments for exported type and constructor; consider avoiding pointer-to-interface internallyPublic types/functions should have doc comments. Also, storing pointers-to-interfaces (
*ResourceVariable) is unidiomatic and complicates nil handling. Internally storing plainResourceVariablevalues would simplify code; you can still return copies as*ResourceVariableto satisfy the generic interface.Apply this doc-only diff:
-type ResourceVariableRepository struct { +// ResourceVariableRepository is an in-memory, thread-safe repository for ResourceVariable. +// It indexes entries by resourceID and key, and returns copies to avoid accidental external mutation. +type ResourceVariableRepository struct { variables map[string]map[string]*ResourceVariable // resourceID -> key -> variable mu sync.RWMutex } func NewResourceVariableRepository() *ResourceVariableRepository { - return &ResourceVariableRepository{variables: make(map[string]map[string]*ResourceVariable)} + // NewResourceVariableRepository constructs an empty repository for resource-scoped variables. + return &ResourceVariableRepository{variables: make(map[string]map[string]*ResourceVariable)} }If you want the internal refactor away from
*ResourceVariable, say the word and I’ll provide a targeted patch.
173-189: Potential scalability: add an ID index for O(1) Get/Exists/DeleteCurrently Get/Exists scan all buckets. If the repository grows, consider maintaining an auxiliary index
idIndex map[string]struct{resourceID, key string}updated in Create/Update/Delete. It’ll make Get/Exists/Delete O(1) and also simplify uniqueness checks.I can provide a patch adding
idIndexwhile preserving thread-safety if you want.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/workspace-engine/pkg/engine/variable/resource_variable_repository.go(1 hunks)apps/workspace-engine/pkg/engine/variable/resource_variable_repository_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/workspace-engine/pkg/engine/variable/resource_variable_repository_test.go
🧰 Additional context used
📓 Path-based instructions (1)
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/engine/variable/resource_variable_repository.go
🧬 Code graph analysis (1)
apps/workspace-engine/pkg/engine/variable/resource_variable_repository.go (2)
apps/workspace-engine/pkg/model/repository.go (1)
Repository(7-14)apps/workspace-engine/pkg/engine/variable/resource_variable.go (1)
ResourceVariable(10-13)
⏰ 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). (2)
- GitHub Check: Lint
- GitHub Check: Typecheck
🔇 Additional comments (5)
apps/workspace-engine/pkg/engine/variable/resource_variable_repository.go (5)
41-58: Get: nil-guards and copy semantics look goodThe nil checks prevent panics and the per-iteration copy avoids the address-of-loop trap. Good defensive handling here.
64-76: GetAllByResourceID: safe iteration and copyingYou correctly guard nil pointer-to-interface and allocate a fresh copy per entry. Looks solid.
78-93: GetByResourceIDAndKey: safe nil handling and copy returnGood: guard both pointer and underlying interface before returning a distinct copy.
22-39: Order of results is nondeterministicMap iteration order is random, so callers of GetAll/GetAllByResourceID should not rely on ordering. If a stable order is desired, sort by (resourceID,key) before returning or document the behavior.
Do any downstream tests or code rely on a specific order? If so, I can add sorting logic.
Also applies to: 60-76
3-9: Imports Verified – No Action RequiredThe module path declared in apps/workspace-engine/go.mod is
workspace-engine, and all existing imports (e.g."workspace-engine/pkg/model") correctly reference this module path. No import changes are needed.
Summary by CodeRabbit
New Features
Tests