refactor: deployment variables #580
Conversation
WalkthroughThis change refactors the handling of deployment variable values by splitting them into distinct "direct" and "reference" types throughout the backend, database schema, API, Node SDK, and frontend. New tables, types, and type guards are introduced, and all related logic, tests, and UI components are updated to reflect this separation. Changes
Sequence Diagram(s)sequenceDiagram
participant UI
participant API
participant DB
participant Utils
UI->>API: Request deployment variables
API->>Utils: getDeploymentVariables(deploymentId)
Utils->>DB: Query deployment variables
Utils->>DB: Query directValues for each variable
Utils->>DB: Query referenceValues for each variable
Utils->>Utils: Decrypt sensitive direct values
Utils->>API: Return variables with directValues, referenceValues, defaultValue
API->>UI: Respond with new structure
sequenceDiagram
participant API
participant Utils
participant DB
API->>Utils: upsertDeploymentVariable(deploymentId, createVariable)
Utils->>DB: Upsert deployment variable
Utils->>DB: Upsert directValues (if any)
Utils->>DB: Upsert referenceValues (if any)
Utils->>DB: Set default value ID
Utils->>DB: Fetch updated variable with values
Utils->>API: Return updated variable
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
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: 7
🧹 Nitpick comments (7)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/page.tsx (1)
37-43: LGTM! Consider the long-term UI strategy for value type distinction.The transformation logic correctly combines
directValuesandreferenceValuesinto a singlevaluesarray for UI compatibility. This is a sensible approach during the refactor transition.However, consider whether the UI should eventually distinguish between direct and reference values for better user experience (e.g., different styling, icons, or grouping).
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/EditVariableDialog.tsx (1)
5-5: Consider using specific imports for better tree-shakingWhile the namespace import works, consider importing only the specific type guards you need for better tree-shaking and clearer dependencies.
-import * as SCHEMA from "@ctrlplane/db/schema"; +import { + isDeploymentVariableValueDirect, + isDeploymentVariableValueReference, +} from "@ctrlplane/db/schema";Then update the usage:
-{SCHEMA.isDeploymentVariableValueDirect(v) && +{isDeploymentVariableValueDirect(v) &&packages/db/src/utils/deployment-variables/upsert-direct-value.ts (1)
117-141: Optimize update logic to avoid unnecessary writesThe current implementation updates all fields even when only some may have changed. Consider checking if the
resourceSelectororsensitiveflag has actually changed before updating.When the value hash matches, you're still updating the
resourceSelectorandsensitivefields. Consider checking if these have actually changed to avoid unnecessary database writes:// In updateExistingDirectVariableValue, check if updates are needed const needsVariableValueUpdate = existingValue.deployment_variable_value.resourceSelector !== updateDirectValue.resourceSelector; const needsDirectValueUpdate = existingValue.deployment_variable_value_direct.sensitive !== updateDirectValue.sensitive; // Only update tables that have changesapps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/route.ts (1)
61-110: Consider consistent return types for better predictability.The function returns
undefinedwhen there's nodefaultValueIdbutnullwhen the reference value is not found. This inconsistency could lead to confusion.Consider returning
nullconsistently:const getDefaultValue = async (db: Tx, variable: schema.DeploymentVariable) => { const { defaultValueId } = variable; - if (defaultValueId == null) return undefined; + if (defaultValueId == null) return null;packages/db/src/utils/deployment-variables/upsert-deployment-variables.ts (2)
39-49: Consider using a custom error class for validation errors.The function throws a generic
Errorwhich makes it harder to distinguish validation errors from other errors in the calling code.Consider creating a custom error class:
class ValidationError extends Error { constructor(message: string) { super(message); this.name = 'ValidationError'; } } // Then in the function: if (numDefault > 1) throw new ValidationError("Only one default value is allowed");
66-80: Consider adding error context for debugging parallel operations.The
Promise.alloperations could fail partially, making debugging difficult. Consider wrapping the operations to add context.Example approach:
const directValuePromises = (directValues ?? []).map(async (dv, index) => { try { const value = await upsertDirectVariableValue(tx, variable.id, dv); if (dv.isDefault) await setDefaultVariableValue(tx, variable.id, value.id); return value; } catch (error) { throw new Error(`Failed to upsert direct value at index ${index}: ${error.message}`); } });packages/db/src/schema/deployment-variables.ts (1)
86-89: Consider enforcing valueHash for sensitive values.The
valueHashfield is optional, but it's unclear when it should be populated. If this field is meant to store a hash of sensitive values for comparison or audit purposes, consider adding a database constraint or application-level validation to ensure it's always populated whensensitiveis true.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (32)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/EditVariableDialog.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/VariableTable.tsx(5 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/VariableValueDropdown.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/page.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/resources/(raw)/[resourceId]/variables/page.tsx(1 hunks)apps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/openapi.ts(5 hunks)apps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/route.ts(4 hunks)e2e/api/entities-builder.ts(3 hunks)e2e/api/entity-fixtures.ts(2 hunks)e2e/api/schema.ts(7 hunks)e2e/tests/api/deployment-variable.spec.ts(5 hunks)e2e/tests/api/release.spec.ts(6 hunks)e2e/tests/api/resource-variables.spec.ts(3 hunks)e2e/tests/api/yaml-import.spec.ts(1 hunks)e2e/tests/api/yaml-import.spec.yaml(2 hunks)openapi.v1.json(6 hunks)packages/api/src/router/deployment-variable/by-deployment-id.ts(2 hunks)packages/api/src/router/deployment-variable/deployment-variable.ts(4 hunks)packages/db/drizzle/0110_lying_virginia_dare.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/package.json(1 hunks)packages/db/src/index.ts(1 hunks)packages/db/src/schema/deployment-variables.ts(2 hunks)packages/db/src/utils/deployment-variables/get-deployment-variables.ts(1 hunks)packages/db/src/utils/deployment-variables/index.ts(1 hunks)packages/db/src/utils/deployment-variables/upsert-deployment-variables.ts(1 hunks)packages/db/src/utils/deployment-variables/upsert-direct-value.ts(1 hunks)packages/db/src/utils/deployment-variables/upsert-reference-value.ts(1 hunks)packages/db/src/utils/index.ts(1 hunks)packages/node-sdk/src/schema.ts(7 hunks)packages/rule-engine/src/manager/variables/db-variable-providers.ts(4 hunks)packages/rule-engine/src/manager/variables/resolve-reference-variable.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{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...
**/*.{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.
packages/db/src/utils/deployment-variables/index.tsapps/webservice/src/app/[workspaceSlug]/(app)/resources/(raw)/[resourceId]/variables/page.tsxe2e/tests/api/resource-variables.spec.tsapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/page.tsxe2e/tests/api/yaml-import.spec.tsapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/VariableValueDropdown.tsxpackages/db/src/utils/deployment-variables/upsert-direct-value.tspackages/rule-engine/src/manager/variables/resolve-reference-variable.tspackages/db/src/index.tspackages/db/src/utils/index.tspackages/api/src/router/deployment-variable/deployment-variable.tsapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/VariableTable.tsxapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/EditVariableDialog.tsxe2e/api/entities-builder.tse2e/tests/api/release.spec.tspackages/db/src/utils/deployment-variables/upsert-deployment-variables.tse2e/api/entity-fixtures.tspackages/db/src/utils/deployment-variables/get-deployment-variables.tspackages/db/src/utils/deployment-variables/upsert-reference-value.tspackages/api/src/router/deployment-variable/by-deployment-id.tsapps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/route.tse2e/api/schema.tse2e/tests/api/deployment-variable.spec.tspackages/db/src/schema/deployment-variables.tspackages/rule-engine/src/manager/variables/db-variable-providers.tsapps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/openapi.tspackages/node-sdk/src/schema.ts
🧠 Learnings (1)
packages/api/src/router/deployment-variable/by-deployment-id.ts (1)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#181
File: packages/api/src/router/deployment.ts:116-131
Timestamp: 2024-10-29T02:04:50.312Z
Learning: In `packages/api/src/router/deployment.ts`, the `list.byDeploymentId` procedure requires multiple database queries due to limitations of the `releaseMatchesCondition` function.
🧬 Code Graph Analysis (5)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/VariableValueDropdown.tsx (2)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/variable-data.ts (1)
VariableValue(3-7)packages/db/src/schema/deployment-variables.ts (1)
DeploymentVariable(51-51)
packages/db/src/utils/deployment-variables/get-deployment-variables.ts (2)
packages/db/src/schema/deployment-variables.ts (2)
DirectDeploymentVariableValue(117-118)DeploymentVariable(51-51)packages/db/src/common.ts (1)
Tx(22-22)
e2e/tests/api/deployment-variable.spec.ts (1)
e2e/tests/fixtures.ts (1)
test(11-26)
packages/rule-engine/src/manager/variables/db-variable-providers.ts (4)
packages/db/src/schema/deployment-variables.ts (3)
DirectDeploymentVariableValue(117-118)ReferenceDeploymentVariableValue(165-166)DeploymentVariableValue(168-170)packages/db/src/utils/deployment-variables/get-deployment-variables.ts (2)
getDeploymentVariables(80-99)getResolvedDirectValue(8-17)packages/rule-engine/src/manager/variables/types.ts (1)
MaybeVariable(9-9)packages/rule-engine/src/manager/variables/resolve-reference-variable.ts (1)
getReferenceVariableValue(8-31)
packages/node-sdk/src/schema.ts (1)
e2e/api/schema.ts (1)
components(892-1421)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Lint
🔇 Additional comments (70)
packages/db/package.json (1)
55-55: LGTM: Dependency addition aligns with refactoring needs.The
ts-is-presentdependency addition follows the established catalog versioning pattern and supports the type presence checks needed for the deployment variable refactoring.packages/db/drizzle/meta/_journal.json (1)
775-781: LGTM: Migration entry follows established pattern.The new migration entry for the deployment variable refactoring is properly structured and follows the existing journal format consistently.
packages/db/src/utils/deployment-variables/index.ts (1)
1-2: LGTM: Clean utility module aggregation.The re-export pattern provides a unified entry point for deployment variable utilities. The .js extensions are appropriate for ESM module compatibility.
packages/db/src/index.ts (1)
9-9: LGTM! Clean consolidation of utility exports.This change effectively consolidates utility exports into a single index module, improving import management without affecting the public API.
packages/rule-engine/src/manager/variables/resolve-reference-variable.ts (1)
12-12: LGTM! Type alignment with refactored schema.The type update from
schema.DeploymentVariableValueReferencetoschema.ReferenceDeploymentVariableValuecorrectly aligns with the refactored schema that separates deployment variable values into distinct direct and reference types.e2e/tests/api/yaml-import.spec.ts (2)
126-129: LGTM! Correct adaptation to new API structure.The test correctly updates from using
valuestodirectValuesarray, maintaining proper test coverage for the refactored deployment variable schema.
133-135: Excellent type safety with the type guard pattern.The addition of the type guard (
"value" in defaultValue!) before accessing thevalueproperty is a robust approach that prevents runtime errors and ensures type safety when working with the potentially polymorphicdefaultValuestructure.packages/db/src/utils/index.ts (1)
1-3: LGTM: Clean utility module aggregation.This new index file properly aggregates utility exports for deployment variables and resource management, supporting the broader refactoring effort with a clean centralized export pattern.
e2e/tests/api/yaml-import.spec.yaml (2)
87-89: LGTM: Consistent with deployment variable refactoring.The changes from
valuestodirectValuesanddefaulttoisDefaultproperly align with the broader refactoring to separate direct and reference variable values and improve property naming clarity.
112-112: LGTM: Simplified policy structure.Removing the unnecessary list wrapper for
versionAnyApprovalsimproves the configuration structure by making it a direct object withrequiredApprovalsCount.e2e/tests/api/release.spec.ts (5)
147-159: LGTM: Enhanced variable structure with explicit properties.The test properly reflects the new API structure with
directValuesarray and explicit properties (sensitive,resourceSelector,isDefault). The addition of a second value object improves test coverage for multiple values scenarios.
368-380: LGTM: Consistent test structure maintained.The variable creation request body correctly uses the new
directValuesstructure with all required properties, maintaining consistency with the updated API schema.
505-517: LGTM: Test coverage expanded with multiple values.The test case properly demonstrates the new structure while testing multiple deployment variable values, which is important for validating the array-based approach.
623-635: LGTM: Consistent API usage throughout tests.All test cases consistently use the new
directValuesstructure, ensuring comprehensive validation of the refactored deployment variable handling.
752-764: LGTM: Complete test suite alignment.The remaining test cases properly implement the new deployment variable structure, ensuring the entire test suite validates the refactored API consistently.
Also applies to: 881-893
e2e/tests/api/resource-variables.spec.ts (3)
340-351: LGTM: Proper reference value structure implementation.The test correctly uses
referenceValuesarray for reference-type deployment variables, eliminating the need for thevalueTypediscriminator since the array placement now determines the value type.
497-508: LGTM: Consistent reference variable testing.The test case properly validates reference variable functionality using the new structure, ensuring that variable references to related resources work correctly with the refactored API.
671-682: LGTM: Complete migration to reference values structure.All reference variable test cases consistently use the new
referenceValuesarray structure, providing comprehensive validation of the refactored deployment variable reference handling.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/EditVariableDialog.tsx (1)
119-126: Type-safe rendering implementation looks good!The use of type guards to conditionally render direct vs reference values is well-implemented and aligns with the schema refactor. The JSON stringification for object values ensures proper display.
packages/db/src/utils/deployment-variables/get-deployment-variables.ts (5)
8-17: LGTM! Good handling of sensitive values.The function correctly differentiates between sensitive and non-sensitive values, and properly handles the decryption process with appropriate type conversions.
19-40: Well-structured database query.The function correctly joins the tables and efficiently merges the results using the spread operator.
42-66: Consistent implementation with proper ordering.Good addition of the
orderByclause to ensure reference values are returned in a predictable order.
68-78: Efficient concurrent data fetching.Excellent use of
Promise.allto fetch direct and reference values in parallel, improving performance.
80-99: Well-implemented main retrieval function.The function properly handles:
- Ordering variables by key for consistent results
- Concurrent processing of variable values
- Correct resolution of the default value from the combined values array
packages/db/src/utils/deployment-variables/upsert-reference-value.ts (4)
7-31: Correct query for finding existing reference values.The function properly uses the unique combination of variableId, reference, and path to identify existing values.
32-58: Proper two-step insertion with transaction safety.The function correctly handles the insertion order required by the foreign key relationship. Since it operates within a transaction context (Tx), atomicity is maintained.
60-89: Consistent update implementation.The function properly updates both related tables within the transaction context, maintaining data consistency.
91-115: Well-implemented upsert pattern.The function correctly implements the upsert logic by checking for existence first, preventing duplicate entries while allowing updates to existing values.
packages/api/src/router/deployment-variable/deployment-variable.ts (2)
248-253: Good refactoring to use centralized upsert logic.The mutation now cleanly delegates to the
upsertDeploymentVariableutility while maintaining queue updates and return values.
248-251:✅ Verification successful
Verify database access for upsertDeploymentVariable.
The mutation no longer uses the
ctxparameter. Please ensure thatupsertDeploymentVariablehas proper database access.
🏁 Script executed:
#!/bin/bash # Description: Check the implementation of upsertDeploymentVariable to ensure it has database access # Search for the upsertDeploymentVariable function implementation ast-grep --pattern 'export const upsertDeploymentVariable = $_' # Also check with ripgrep for more context rg -A 10 "export.*upsertDeploymentVariable" --type tsLength of output: 6188
No DB access issue – upsertDeploymentVariable uses db.transaction internally
Verified that the
upsertDeploymentVariablefunction (packages/db/src/utils/deployment-variables/upsert-deployment-variables.ts) wraps its logic indb.transaction(...), so it already has full database access without needing thectxparameter in the mutation. Removingctxhere is safe.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/VariableTable.tsx (5)
13-13: LGTM: Centralized schema import added.The addition of the schema import enables the use of centralized type guard functions, which is a good practice for maintaining type safety and consistency across the codebase.
138-142: LGTM: Consistent type guard usage replaces direct property checks.The replacement of
value.valueType === "direct"withschema.isDeploymentVariableValueDirect(value)centralizes type checking logic and improves maintainability. This pattern is applied consistently throughout the component for icon rendering, tooltip content, and conditional value display.Also applies to: 146-148, 153-157, 211-215, 219-221, 226-230
159-159: LGTM: Simplified path handling.The removal of the nullish coalescing operator (
??) fromvalue.pathusage suggests the schema now guarantees this property is always present, which simplifies the code.Also applies to: 232-232
287-291: LGTM: Type guard determines valueType prop.Using the type guard to dynamically determine the
valueTypeprop value maintains consistency with the new type checking approach and ensures the correct type is passed to child components.
413-418: LGTM: Search functionality updated for new schema.The search keys now correctly serialize either the direct
valueor the referencereferenceproperty based on the type guard, maintaining search functionality while adapting to the new variable value structure.e2e/api/entity-fixtures.ts (3)
23-40: LGTM: Well-structured directValues schema.The
directValuesarray schema correctly defines:
- Nullable union type for
valuesupporting string, number, boolean, object, and array types- Optional
sensitiveboolean flag- Nullable
resourceSelectorfor flexibility- Optional
isDefaultboolean for marking default valuesThe schema structure aligns with the new deployment variable value model.
41-59: LGTM: Comprehensive referenceValues schema.The
referenceValuesarray schema properly defines:
- Nullable
resourceSelectorfor flexible resource targeting- Required
referencestring for the reference identifier- Required
patharray of strings for property access- Nullable
defaultValuewith the same union type as direct values- Optional
isDefaultboolean for default value markingThis structure supports the reference-based variable resolution pattern.
112-113: LGTM: PolicyFixture simplified.The change from an optional array of objects to an optional single object with
requiredApprovalsCountsimplifies the policy approval structure, which appears to be a separate but related improvement.packages/db/drizzle/0110_lying_virginia_dare.sql (4)
1-8: LGTM: Well-designed direct values table.The
deployment_variable_value_directtable properly includes:
- UUID primary key with automatic generation
- Foreign key to the main variable value table
- JSONB
valuefield for flexible data storagevalue_hashfor potential indexing/comparison optimizationsensitiveboolean flag with appropriate default- Unique constraint on
variable_value_idensuring one-to-one relationshipThe structure supports efficient storage and querying of direct variable values.
10-17: LGTM: Comprehensive reference values table.The
deployment_variable_value_referencetable effectively captures:
- UUID primary key with automatic generation
- Foreign key to the main variable value table
- Required
referenceandpathfields for resource property access- Optional JSONB
default_valuefor fallback scenarios- Unique constraint ensuring one-to-one relationship with variable values
This design supports the reference-based variable resolution pattern.
19-20: LGTM: Proper foreign key constraints with cascade delete.The foreign key constraints correctly reference the parent table with
ON DELETE cascadeensuring data integrity when variable values are removed. TheON UPDATE no actionis appropriate for UUID primary keys.
21-27: LGTM: Clean removal of legacy schema elements.The migration properly removes the deprecated columns (
value_type,value,sensitive,reference,path,default_value) and thevalue_typeenum, completing the transition to the new separated table structure.e2e/api/entities-builder.ts (3)
44-58: LGTM: Cache interface updated for separated value types.The interface correctly defines:
directValuesarray withid,value,sensitive,resourceSelector, andisDefaultpropertiesreferenceValuesarray withid,resourceSelector,reference,path,defaultValue, andisDefaultpropertiesBoth arrays are optional, maintaining flexibility for deployments without variables. The structure aligns with the new API schema.
316-323: LGTM: Proper API request body construction.The request body construction correctly:
- Maps over
directValuesandreferenceValuesarrays separately- Ensures
resourceSelectorfields are explicitly set tonullwhen undefined- Maintains the original structure while adapting to the new schema requirements
This handles the API contract properly and prevents undefined values from being sent.
337-350: LGTM: Robust response handling with UUID assignment.The response processing effectively:
- Destructures
directValuesandreferenceValuesfrom the API response- Assigns UUIDs to entries missing an
idusing faker for test consistency- Updates the cache with the properly structured variable data
The defensive UUID assignment ensures test stability even if the API response format varies.
e2e/tests/api/deployment-variable.spec.ts (2)
130-131: Good use of find() for test resilience!Using
find()to locate the variable by key instead of array indexing makes the test more robust against changes in the order of returned variables.
264-391: Comprehensive test coverage for variable updates!This test thoroughly validates the update functionality including resourceSelector changes for both direct and reference values.
apps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/route.ts (1)
49-50: Clean refactor using centralized helper!The GET handler is now much simpler by delegating to
getDeploymentVariables, improving maintainability.packages/api/src/router/deployment-variable/by-deployment-id.ts (4)
17-25: LGTM!The helper function correctly fetches release targets with their associated resources.
48-50: Verify the sensitive value masking behavior.The code masks sensitive direct values as
"***". Please confirm this is the intended behavior for UI display purposes and that the actual values are properly handled when needed for deployment operations.
67-106: Well-structured helper function!The function correctly processes both direct and reference values, identifies unmatched resources, and properly assigns the default value to them.
108-128: Clean implementation!The procedure follows a clear flow and properly leverages the helper functions to resolve deployment variables by resource.
apps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/openapi.ts (1)
11-132: Well-structured schema refactoring!The separation of direct and reference variable values into distinct schemas with "WithId" variants provides better type safety and clarity. The renaming from "default" to "isDefault" improves semantic clarity.
packages/rule-engine/src/manager/variables/db-variable-providers.ts (1)
96-104: Clear documentation of variable precedence!The comment clearly explains the resolution order, which helps maintainability.
packages/db/src/schema/deployment-variables.ts (3)
102-102: Clarify the purpose of null values in direct deployment variable values.The schema allows
valueto be null for direct values, which seems counterintuitive. Consider whether null values should be handled differently (e.g., by not creating a value record, or using reference values with null defaults).
172-179: Clean type guard implementation.The simplified type guards using property presence checks are a good improvement over discriminator fields. This approach is more intuitive and reduces complexity.
207-216: Well-structured schema update for separated value types.The schema correctly reflects the new architecture with separate
directValuesandreferenceValuesarrays. Making both optional provides good flexibility for variable creation scenarios.e2e/api/schema.ts (2)
913-953: Correct OpenAPI schema generation for the new variable value types.The auto-generated schemas properly reflect the backend's architectural change to separate direct and reference deployment variable values. The addition of ID fields to the value types enables better tracking and management.
1316-1336: Good addition of concurrency control to policies.The optional
concurrencyfield allows for better control over parallel policy executions while maintaining backward compatibility by making it nullable.openapi.v1.json (6)
4769-4784: Correctly renamed base variable value schema.The new
BaseDeploymentVariableValuereplaces the old base schema, makesresourceSelectorrequired (nullable), and renames thedefaultflag toisDefault, improving clarity and aligning with the direct/reference split.
4785-4822: Direct value schema properly updated.
DirectDeploymentVariableValuenow extends the base type, removes thevalueTypediscriminator, and allows a nullablevalue, accurately reflecting the new design for direct variable values.
4823-4841: Added unique IDs for direct value entries.The
DirectDeploymentVariableValueWithIdschema correctly layers on a required UUIDid, enabling unique identification of each direct value record.
4842-4886: Reference value schema aligns with refactor.
ReferenceDeploymentVariableValueproperly extends the base, drops the old discriminator, and definespath,reference, and nullabledefaultValue, fully supporting the separated reference values model.
4887-4905: Added unique IDs for reference value entries.The
ReferenceDeploymentVariableValueWithIdschema correctly introduces a required UUIDid, ensuring reference values can be uniquely tracked.
4906-4954: DeploymentVariable schema updated to separate value lists.Splitting into
directValuesandreferenceValuesarrays and centralizingdefaultValueas a union of the two WithId schemas creates a clearer API contract and enhances type safety.packages/node-sdk/src/schema.ts (5)
913-938: LGTM! Well-structured type hierarchy for deployment variable values.The refactoring properly separates direct and reference variable values with:
- Clear inheritance from
BaseDeploymentVariableValue- Appropriate nullable types for
resourceSelector,value, anddefaultValue- Consistent ID extensions for both direct and reference types
- Required
sensitivefield for direct values
944-948: LGTM! Consistent schema update for DeploymentVariable.The changes correctly implement the separation of variable values:
- Distinct arrays for
directValuesandreferenceValuesusing the WithId variants- Flexible
defaultValuethat can be either type- Maintains type safety with proper union types
1189-1190: LGTM! Standard soft-delete implementation.The addition of nullable
deletedAttimestamp follows best practices for soft-delete functionality.
1315-1316: LGTM! Clean implementation of policy concurrency support.The nullable integer type for
PolicyConcurrencyappropriately handles both concurrency limits and unlimited cases (null).Also applies to: 1334-1334
2022-2023: LGTM! API operations correctly updated to match schema changes.The changes maintain consistency across all operations:
createDeploymentVariableproperly uses the new separated value arrays- Policy operations (
updatePolicy,upsertPolicy) include the new concurrency fieldThis ensures the API contract is properly maintained throughout the SDK.
Also applies to: 2653-2653, 2756-2756
| <TableRow key={v.key}> | ||
| <TableCell>{v.key}</TableCell> | ||
| <TableCell>{String(v.value.value)}</TableCell> | ||
| <TableCell></TableCell> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve user experience with empty value cell.
The deployment variable value cell is now completely empty, which could confuse users. Consider:
- Adding a placeholder message explaining the refactoring
- Implementing proper value display using the new direct/reference structure
- Adding a "Not available" or similar indicator
This appears to be an incomplete implementation that needs user-facing improvements.
- <TableCell></TableCell>
+ <TableCell className="text-muted-foreground">
+ <span className="italic">Value display pending refactor</span>
+ </TableCell>📝 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.
| <TableCell></TableCell> | |
| <TableCell className="text-muted-foreground"> | |
| <span className="italic">Value display pending refactor</span> | |
| </TableCell> |
🤖 Prompt for AI Agents
In
apps/webservice/src/app/[workspaceSlug]/(app)/resources/(raw)/[resourceId]/variables/page.tsx
at line 252, the TableCell for deployment variable value is empty, which can
confuse users. Update this cell to display a meaningful placeholder such as "Not
available" or a message explaining the refactoring. Additionally, implement
logic to properly render the variable value using the new direct/reference
structure to improve clarity and user experience.
There was a problem hiding this comment.
will address in separate PR
There was a problem hiding this comment.
@adityachoudhari26 That sounds like a good approach to keep this PR focused on the refactoring!
Would you like me to open a GitHub issue to track implementing proper deployment variable value display using the new direct/reference structure? This would help ensure the UI improvement doesn't get lost.
| const EditVariableValueDialog: React.FC<{ | ||
| value: VariableValue; | ||
| variable: schema.DeploymentVariable; | ||
| onClose: () => void; | ||
| children: React.ReactNode; | ||
| }> = ({ value, variable, children, onClose }) => { | ||
| const [open, setOpen] = useState(false); | ||
| const update = api.deployment.variable.value.update.useMutation(); | ||
| const router = useRouter(); | ||
|
|
||
| const form = useForm({ | ||
| schema: schema.updateDeploymentVariableValue, | ||
| defaultValues: { | ||
| ...value, | ||
| default: variable.defaultValueId === value.id, | ||
| }, | ||
| }); | ||
|
|
||
| const onSubmit = form.handleSubmit((data) => | ||
| update | ||
| .mutateAsync({ id: value.id, data }) | ||
| .then(() => router.refresh()) | ||
| .then(onClose), | ||
| ); | ||
|
|
||
| return ( | ||
| <Dialog | ||
| open={open} | ||
| onOpenChange={(open) => { | ||
| setOpen(open); | ||
| if (!open) onClose(); | ||
| }} | ||
| > | ||
| <DialogTrigger asChild>{children}</DialogTrigger> | ||
| <DialogContent className="min-w-[1000px]"> | ||
| <DialogHeader> | ||
| <DialogTitle>Edit variable value</DialogTitle> | ||
| </DialogHeader> | ||
|
|
||
| <Form {...form}> | ||
| <form onSubmit={onSubmit} className="space-y-3"> | ||
| <FormField | ||
| control={form.control} | ||
| name="value" | ||
| render={({ field: { value, onChange } }) => ( | ||
| <FormItem> | ||
| <FormLabel>Value</FormLabel> | ||
| <FormControl> | ||
| <> | ||
| {variable.config?.type === "string" && ( | ||
| <VariableStringInput | ||
| {...variable.config} | ||
| value={String(value)} | ||
| onChange={onChange} | ||
| /> | ||
| )} | ||
| {variable.config?.type === "choice" && ( | ||
| <VariableChoiceSelect | ||
| {...variable.config} | ||
| value={String(value)} | ||
| onSelect={onChange} | ||
| /> | ||
| )} | ||
| {variable.config?.type === "boolean" && ( | ||
| <VariableBooleanInput | ||
| value={value === "" ? null : Boolean(value)} | ||
| onChange={onChange} | ||
| /> | ||
| )} | ||
| {variable.config?.type === "number" && ( | ||
| <Input | ||
| type="number" | ||
| value={Number(value)} | ||
| onChange={(e) => onChange(e.target.valueAsNumber)} | ||
| /> | ||
| )} | ||
| </> | ||
| </FormControl> | ||
| <FormMessage /> | ||
| </FormItem> | ||
| )} | ||
| /> | ||
|
|
||
| <FormField | ||
| control={form.control} | ||
| name="resourceSelector" | ||
| render={({ field: { value, onChange } }) => ( | ||
| <FormItem> | ||
| <FormLabel>Resource selector</FormLabel> | ||
| <FormControl> | ||
| <ResourceConditionRender | ||
| condition={value ?? defaultCondition} | ||
| onChange={onChange} | ||
| /> | ||
| </FormControl> | ||
| <FormMessage /> | ||
| </FormItem> | ||
| )} | ||
| /> | ||
|
|
||
| <FormField | ||
| control={form.control} | ||
| name="default" | ||
| render={({ field: { value, onChange } }) => ( | ||
| <FormItem> | ||
| <div className="flex items-center gap-2"> | ||
| <FormLabel>Default</FormLabel> | ||
| <FormControl> | ||
| <Switch checked={value} onCheckedChange={onChange} /> | ||
| </FormControl> | ||
| </div> | ||
| </FormItem> | ||
| )} | ||
| /> | ||
|
|
||
| <DialogFooter> | ||
| <Button | ||
| variant="outline" | ||
| type="button" | ||
| onClick={() => form.setValue("resourceSelector", null)} | ||
| > | ||
| Clear selector | ||
| </Button> | ||
| <div className="flex-grow" /> | ||
| <Button type="submit">Save</Button> | ||
| </DialogFooter> | ||
| </form> | ||
| </Form> | ||
| </DialogContent> | ||
| </Dialog> | ||
| ); | ||
| }> = () => { | ||
| return null; | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Non-functional component still being used in the UI
The EditVariableValueDialog component has been replaced with a stub that returns null, but it's still being used in the dropdown menu (lines 93-105). This creates a confusing user experience where the "Edit" button is visible but does nothing when clicked.
Either remove the usage of this component from the dropdown menu or add a TODO comment explaining when the implementation will be restored:
-}> = () => {
- return null;
+}> = () => {
+ // TODO: Implement after variable value refactoring is complete
+ return null;
};Or remove the edit option entirely:
<DropdownMenuContent>
<DropdownMenuGroup>
- <EditVariableValueDialog
- value={value}
- variable={variable}
- onClose={() => setOpen(false)}
- >
- <DropdownMenuItem
- onSelect={(e) => e.preventDefault()}
- className="flex items-center gap-2"
- >
- <IconPencil className="h-4 w-4" />
- Edit
- </DropdownMenuItem>
- </EditVariableValueDialog>
<DeleteVariableValueDialog🏁 Script executed:
#!/bin/bash
# Locate definition and usages of EditVariableValueDialog
rg "EditVariableValueDialog" -n apps/webservice/src/appLength of output: 668
Address non-functional EditVariableValueDialog usage
The EditVariableValueDialog is a stub returning null but still rendered in the dropdown (lines 93–105), so the “Edit” option does nothing and will confuse users.
Please either implement the dialog (or add a clear TODO) or remove its usage entirely:
• File: apps/webservice/src/app/[workspaceSlug]/…/VariableValueDropdown.tsx
– Definition at lines 28–35
– Usage at lines 93–105
Option 1: Add a TODO in the stub
const EditVariableValueDialog: React.FC<{
value: VariableValue;
variable: schema.DeploymentVariable;
onClose: () => void;
children: React.ReactNode;
}> = () => {
+ // TODO: implement EditVariableValueDialog after refactoring variable values
return null;
};Option 2: Remove the edit menu item
<DropdownMenuContent>
<DropdownMenuGroup>
- <EditVariableValueDialog
- value={value}
- variable={variable}
- onClose={() => setOpen(false)}
- >
- <DropdownMenuItem
- onSelect={(e) => e.preventDefault()}
- className="flex items-center gap-2"
- >
- <IconPencil className="h-4 w-4" />
- Edit
- </DropdownMenuItem>
- </EditVariableValueDialog>
<DeleteVariableValueDialog
value={value}
variable={variable}📝 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.
| const EditVariableValueDialog: React.FC<{ | |
| value: VariableValue; | |
| variable: schema.DeploymentVariable; | |
| onClose: () => void; | |
| children: React.ReactNode; | |
| }> = ({ value, variable, children, onClose }) => { | |
| const [open, setOpen] = useState(false); | |
| const update = api.deployment.variable.value.update.useMutation(); | |
| const router = useRouter(); | |
| const form = useForm({ | |
| schema: schema.updateDeploymentVariableValue, | |
| defaultValues: { | |
| ...value, | |
| default: variable.defaultValueId === value.id, | |
| }, | |
| }); | |
| const onSubmit = form.handleSubmit((data) => | |
| update | |
| .mutateAsync({ id: value.id, data }) | |
| .then(() => router.refresh()) | |
| .then(onClose), | |
| ); | |
| return ( | |
| <Dialog | |
| open={open} | |
| onOpenChange={(open) => { | |
| setOpen(open); | |
| if (!open) onClose(); | |
| }} | |
| > | |
| <DialogTrigger asChild>{children}</DialogTrigger> | |
| <DialogContent className="min-w-[1000px]"> | |
| <DialogHeader> | |
| <DialogTitle>Edit variable value</DialogTitle> | |
| </DialogHeader> | |
| <Form {...form}> | |
| <form onSubmit={onSubmit} className="space-y-3"> | |
| <FormField | |
| control={form.control} | |
| name="value" | |
| render={({ field: { value, onChange } }) => ( | |
| <FormItem> | |
| <FormLabel>Value</FormLabel> | |
| <FormControl> | |
| <> | |
| {variable.config?.type === "string" && ( | |
| <VariableStringInput | |
| {...variable.config} | |
| value={String(value)} | |
| onChange={onChange} | |
| /> | |
| )} | |
| {variable.config?.type === "choice" && ( | |
| <VariableChoiceSelect | |
| {...variable.config} | |
| value={String(value)} | |
| onSelect={onChange} | |
| /> | |
| )} | |
| {variable.config?.type === "boolean" && ( | |
| <VariableBooleanInput | |
| value={value === "" ? null : Boolean(value)} | |
| onChange={onChange} | |
| /> | |
| )} | |
| {variable.config?.type === "number" && ( | |
| <Input | |
| type="number" | |
| value={Number(value)} | |
| onChange={(e) => onChange(e.target.valueAsNumber)} | |
| /> | |
| )} | |
| </> | |
| </FormControl> | |
| <FormMessage /> | |
| </FormItem> | |
| )} | |
| /> | |
| <FormField | |
| control={form.control} | |
| name="resourceSelector" | |
| render={({ field: { value, onChange } }) => ( | |
| <FormItem> | |
| <FormLabel>Resource selector</FormLabel> | |
| <FormControl> | |
| <ResourceConditionRender | |
| condition={value ?? defaultCondition} | |
| onChange={onChange} | |
| /> | |
| </FormControl> | |
| <FormMessage /> | |
| </FormItem> | |
| )} | |
| /> | |
| <FormField | |
| control={form.control} | |
| name="default" | |
| render={({ field: { value, onChange } }) => ( | |
| <FormItem> | |
| <div className="flex items-center gap-2"> | |
| <FormLabel>Default</FormLabel> | |
| <FormControl> | |
| <Switch checked={value} onCheckedChange={onChange} /> | |
| </FormControl> | |
| </div> | |
| </FormItem> | |
| )} | |
| /> | |
| <DialogFooter> | |
| <Button | |
| variant="outline" | |
| type="button" | |
| onClick={() => form.setValue("resourceSelector", null)} | |
| > | |
| Clear selector | |
| </Button> | |
| <div className="flex-grow" /> | |
| <Button type="submit">Save</Button> | |
| </DialogFooter> | |
| </form> | |
| </Form> | |
| </DialogContent> | |
| </Dialog> | |
| ); | |
| }> = () => { | |
| return null; | |
| }; | |
| const EditVariableValueDialog: React.FC<{ | |
| value: VariableValue; | |
| variable: schema.DeploymentVariable; | |
| onClose: () => void; | |
| children: React.ReactNode; | |
| }> = () => { | |
| // TODO: implement EditVariableValueDialog after refactoring variable values | |
| return null; | |
| }; |
🤖 Prompt for AI Agents
In
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/VariableValueDropdown.tsx
around lines 28 to 35 and usage at lines 93 to 105, the EditVariableValueDialog
component is a stub returning null but still used in the dropdown menu, causing
the Edit button to do nothing. To fix this, either add a clear TODO comment
inside the EditVariableValueDialog stub indicating when the implementation will
be restored or remove the edit menu item and all references to
EditVariableValueDialog from the dropdown menu to avoid confusing users.
There was a problem hiding this comment.
will address in separate PR
There was a problem hiding this comment.
@adityachoudhari26 Understood! That makes sense to keep this PR focused on the variable refactoring.
Would you like me to create a GitHub issue to track the EditVariableValueDialog fix as a follow-up task? I can include the details about the non-functional component and reference this PR and comment.
| const getValueAndHash = ( | ||
| value: string | number | boolean | object | null, | ||
| sensitive: boolean, | ||
| ) => { | ||
| const stringifiedValue = | ||
| typeof value === "object" ? JSON.stringify(value) : String(value); | ||
|
|
||
| const valueHash = createHash("md5") | ||
| .update(stringifiedValue, "utf8") | ||
| .digest("hex"); | ||
|
|
||
| const maybeEncryptedValue = sensitive | ||
| ? variablesAES256().encrypt(stringifiedValue) | ||
| : value; | ||
|
|
||
| return { value: maybeEncryptedValue, valueHash }; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Security concern with MD5 and null value handling issue
-
MD5 is cryptographically broken and shouldn't be used for security purposes. While it might be acceptable for deduplication, consider using SHA-256 for better collision resistance.
-
The null value handling on line 38 will convert
nullto the string"null", which may not be the intended behavior.
-import { createHash } from "crypto";
+import { createHash } from "crypto";
const getValueAndHash = (
value: string | number | boolean | object | null,
sensitive: boolean,
) => {
const stringifiedValue =
- typeof value === "object" ? JSON.stringify(value) : String(value);
+ value === null
+ ? "null"
+ : typeof value === "object"
+ ? JSON.stringify(value)
+ : String(value);
- const valueHash = createHash("md5")
+ const valueHash = createHash("sha256")
.update(stringifiedValue, "utf8")
.digest("hex");Consider if you really want null to be stringified as "null" or if it should be handled differently.
📝 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.
| const getValueAndHash = ( | |
| value: string | number | boolean | object | null, | |
| sensitive: boolean, | |
| ) => { | |
| const stringifiedValue = | |
| typeof value === "object" ? JSON.stringify(value) : String(value); | |
| const valueHash = createHash("md5") | |
| .update(stringifiedValue, "utf8") | |
| .digest("hex"); | |
| const maybeEncryptedValue = sensitive | |
| ? variablesAES256().encrypt(stringifiedValue) | |
| : value; | |
| return { value: maybeEncryptedValue, valueHash }; | |
| }; | |
| import { createHash } from "crypto"; | |
| const getValueAndHash = ( | |
| value: string | number | boolean | object | null, | |
| sensitive: boolean, | |
| ) => { | |
| const stringifiedValue = | |
| value === null | |
| ? "null" | |
| : typeof value === "object" | |
| ? JSON.stringify(value) | |
| : String(value); | |
| const valueHash = createHash("sha256") | |
| .update(stringifiedValue, "utf8") | |
| .digest("hex"); | |
| const maybeEncryptedValue = sensitive | |
| ? variablesAES256().encrypt(stringifiedValue) | |
| : value; | |
| return { value: maybeEncryptedValue, valueHash }; | |
| }; |
🤖 Prompt for AI Agents
In packages/db/src/utils/deployment-variables/upsert-direct-value.ts around
lines 33 to 49, replace the use of MD5 hashing with SHA-256 to improve security
and collision resistance. Also, modify the handling of null values so that null
is not converted to the string "null"; instead, handle null explicitly, for
example by returning null or an empty string before hashing and encryption,
depending on the intended logic.
There was a problem hiding this comment.
we're only using it for deduplication
There was a problem hiding this comment.
@adityachoudhari26 Thanks for the clarification! You're right that for deduplication purposes, MD5 can be acceptable since the primary concerns are performance and basic collision avoidance rather than cryptographic security. The actual security is handled separately by the AES256 encryption for sensitive values.
While SHA-256 would provide better collision resistance even for deduplication, I understand the performance trade-offs, especially if you're processing many variable values.
However, I'd still like to highlight the null value handling on line 38: String(null) will convert null to the string "null". Is this the intended behavior for your deduplication logic, or should null values be handled differently?
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#580
File: packages/db/src/utils/deployment-variables/upsert-direct-value.ts:33-49
Timestamp: 2025-06-02T17:54:43.093Z
Learning: In the ctrlplane codebase, MD5 hashing is acceptable for deduplication purposes in deployment variable values, as the actual security encryption is handled separately via AES256. The MD5 hash is used purely for checking if a variable value with the same content already exists to avoid duplicates.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
thats fine, because we want every null value to be treated the same, so they will still all hash to the same thing
There was a problem hiding this comment.
@adityachoudhari26 Perfect! That makes complete sense for the deduplication logic. Having all null values consistently hash to the same value ensures proper deduplication behavior across deployment variables. Thanks for explaining the design rationale - that resolves my concern about the null value handling.
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#580
File: packages/db/src/utils/deployment-variables/upsert-direct-value.ts:33-49
Timestamp: 2025-06-02T17:59:53.630Z
Learning: In the ctrlplane codebase's deployment variable deduplication logic, null values are intentionally converted to the string "null" using String(null) so that all null values hash to the same value for consistent deduplication behavior across deployment variables.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| .mutation(async ({ ctx, input }) => { | ||
| const { variableId, data } = input; | ||
| const variable = await ctx.db.query.deploymentVariable.findFirst({ | ||
| where: eq(deploymentVariable.id, variableId), | ||
| }); | ||
| if (variable == null) throw new Error("Variable not found"); | ||
| const valueInsert = { ...data, variableId }; | ||
| console.log(valueInsert); | ||
| // const value = await upsertVariableValue(ctx.db, valueInsert); | ||
| // await updateDeploymentVariableQueue.add(variableId, variable); | ||
| // return value; | ||
| }), |
There was a problem hiding this comment.
Critical: Mutation is non-functional with commented out logic.
The value.create mutation currently only logs to console and doesn't perform any database operations or return a value. This breaks the API contract and will cause client errors.
The commented out code suggests incomplete refactoring. Either:
- Complete the refactoring to use the new upsert utilities
- Restore the previous working implementation
- Remove this mutation if it's no longer needed
🤖 Prompt for AI Agents
In packages/api/src/router/deployment-variable/deployment-variable.ts lines 54
to 65, the mutation currently only logs data and does not perform any database
operations or return a value, breaking the API contract. To fix this, either
uncomment and properly integrate the upsertVariableValue call and the
updateDeploymentVariableQueue addition to complete the mutation logic, ensuring
it performs the intended database update and returns the updated value, or if
refactoring is incomplete, restore the previous working implementation or remove
the mutation if it is no longer needed.
There was a problem hiding this comment.
will do in separate PR
There was a problem hiding this comment.
@adityachoudhari26 Sounds good! Would you like me to create a GitHub issue to track fixing the non-functional value.create mutation as a follow-up task? This would help ensure it doesn't get forgotten during the refactoring process.
| if (isDirect) expect(receivedDefaultValue!.value).toBe(valueB); | ||
| }); | ||
|
|
||
| test("shoudl fail if more than one default value is provided", async ({ |
There was a problem hiding this comment.
Fix typo in test name.
- test("shoudl fail if more than one default value is provided", async ({
+ test("should fail if more than one default value is provided", async ({📝 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.
| test("shoudl fail if more than one default value is provided", async ({ | |
| test("should fail if more than one default value is provided", async ({ |
🤖 Prompt for AI Agents
In e2e/tests/api/deployment-variable.spec.ts at line 218, there is a typo in the
test name "shoudl fail if more than one default value is provided". Correct the
typo by changing "shoudl" to "should" to fix the test name.
| default: { type: "boolean" }, | ||
| isDefault: { type: "boolean" }, | ||
| }, | ||
| required: ["resourceSelector"], |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify backward compatibility for required resourceSelector.
Making resourceSelector a required field in BaseDeploymentVariableValue could be a breaking change if existing data or API clients expect it to be optional. Please ensure this change is compatible with existing deployments or that proper migration is in place.
🏁 Script executed:
#!/bin/bash
# Check if there are any deployment variable values without resourceSelector in the codebase
rg -A 5 "deploymentVariableValue.*resourceSelector.*null|undefined" --type tsLength of output: 67365
🏁 Script executed:
#!/bin/bash
# List all occurrences of resourceSelector in e2e API tests to identify where null/undefined is still used
rg "resourceSelector" -n e2e/tests/apiLength of output: 7516
ResourceSelector Required―Tests and Specs Need Updates
Adding resourceSelector as a required field in BaseDeploymentVariableValue is a breaking change: numerous e2e tests and YAML specs still pass null or undefined. Please update all affected tests and specs to include a valid resourceSelector or adjust the schema to provide a default.
Affected locations include, but are not limited to:
- e2e/tests/api/release.spec.ts (many
resourceSelector: nullentries) - e2e/tests/api/deployment-variable.spec.ts (
resourceSelector: null/ expectations on null) - e2e/tests/api/environments.spec.ts (
resourceSelector: undefined/.toBeNull()checks) - e2e/tests/api/deployments.spec.ts and other YAML-based spec files under e2e/tests/api
Ensure each test or spec supplies a valid selector or that the API gracefully handles missing selectors before merging.
🤖 Prompt for AI Agents
In
apps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/openapi.ts
at line 21, making resourceSelector required in BaseDeploymentVariableValue is a
breaking change. To fix this, update all affected e2e tests and YAML specs that
currently pass null or undefined for resourceSelector by providing a valid
resourceSelector value. Alternatively, modify the schema to supply a default
resourceSelector or adjust the API to handle missing selectors gracefully,
ensuring backward compatibility before merging.
There was a problem hiding this comment.
its a required field, but its marked as nullable. so resourceSelector: null is valid
There was a problem hiding this comment.
@adityachoudhari26 You're absolutely right! Thank you for the clarification.
I misunderstood the distinction between optional and required+nullable fields. In this schema:
required: ["resourceSelector"]means the field must be present in the objectnullable: truemeans the field's value can explicitly benull
So resourceSelector: null is indeed valid - the field is present (satisfying the required constraint) but has a null value (which is allowed). This maintains backward compatibility with existing tests and API usage.
This is not a breaking change as I initially thought. The schema correctly enforces that the field must be included while allowing null values.
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#580
File: apps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/openapi.ts:21-21
Timestamp: 2025-06-02T17:55:33.871Z
Learning: In OpenAPI schemas, a field marked as both "required" and "nullable" means the field must be present in the request/response object, but its value can be explicitly set to null. This is different from optional fields where the field can be omitted entirely. For example, `resourceSelector: null` is valid when resourceSelector is required+nullable, but would be invalid if resourceSelector was required+non-nullable.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Chores