init value selection #425
init value selection #425
Conversation
WalkthroughThis pull request refactors the deployment variable schema for improved readability by streamlining the syntax and adding a new inferred type export. It also introduces several new configuration files: an ESLint configuration file and a TypeScript configuration file, and a new Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant RV as ResourceVariable
participant DV as DeploymentVariable
participant VS as VariableSet
Caller->>RV: getResourceVariable(db, resourceId, key)
alt Resource variable exists
RV-->>Caller: value
else Resource variable missing
Caller->>DV: getDeploymentVariableValue(db, resourceId, deploymentId, key)
alt Deployment variable exists
DV-->>Caller: value
else Deployment variable missing
Caller->>VS: getVariableSetValue(db, environmentId, key)
VS-->>Caller: value or null
end
end
Possibly related PRs
Suggested reviewers
Poem
✨ 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:
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: 1
🧹 Nitpick comments (3)
packages/deployment-variables/src/generate-releases.ts (3)
11-21: Consider returning only the needed property.
getResourceVariablecurrently returns the entire resource-variable row, though only thevaluecolumn is used downstream. Returning a more specific structure (e.g., just the value) could simplify your data flow and make the function’s intent clearer.- .then(takeFirstOrNull); + .then((row) => { + const result = takeFirstOrNull(row); + return result ? result.value : null; + });
31-73: Enhance efficiency when fetching deployment variable values.The current approach sorts all values, then iterates and runs a query for each to match a resource. This can become inefficient at scale. Consider a single, more selective query (e.g., a joined query) to match the resource and resource selector at once, reducing repeated lookups.
126-146: Examine fallback strategy and incorporate default handling.When none of the three checks yields a value, this function returns
null. If a fallback or default is expected, consider returning a more descriptive result or throwing an error to clearly indicate an unassigned key, based on your business logic.
📜 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 (5)
packages/db/src/schema/deployment-variables.ts(1 hunks)packages/deployment-variables/eslint.config.js(1 hunks)packages/deployment-variables/package.json(1 hunks)packages/deployment-variables/src/generate-releases.ts(1 hunks)packages/deployment-variables/tsconfig.json(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/schema/deployment-variables.tspackages/deployment-variables/src/generate-releases.ts
🧬 Code Definitions (1)
packages/deployment-variables/src/generate-releases.ts (1)
packages/db/src/schema/deployment-variables.ts (1)
deploymentVariableValue(57-76)
🪛 GitHub Actions: CI
packages/deployment-variables/package.json
[error] 1-1: Failed to add workspace '@ctrlplane/rule-engine', it already exists at 'packages/rule-engine/package.json'.
🔇 Additional comments (5)
packages/deployment-variables/eslint.config.js (1)
1-13: LGTM! Clear and well-structured ESLint configuration.The configuration properly extends the base config and adds specific customizations. Good job disabling the
@typescript-eslint/require-awaitrule if your code needs to use async functions that don't necessarily contain await statements.packages/deployment-variables/tsconfig.json (1)
1-11: Well-structured TypeScript configurationThe TypeScript configuration extends the base configuration appropriately and includes all necessary settings for proper compilation. The inclusion of incremental builds and a specific build info file location are good optimizations for development.
packages/db/src/schema/deployment-variables.ts (1)
84-88: Excellent code refactoring for improved readabilityThe code has been streamlined by simplifying the chained method calls and adding a useful type export. The new
CreateDeploymentVariableValuetype will improve type safety when working with the schema across the codebase.packages/deployment-variables/src/generate-releases.ts (2)
1-2: Imports look good.The imports from
@ctrlplane/dband@ctrlplane/db/schemaare clear and match the usage throughout this file.
82-106: Validate handling of multiple environment variable set entries.
getVariableSetValueretrieves only the very first matching value (ordered by JSON ascending). Ensure that returning just one entry (due to.limit(1)) is sufficient for your business requirements, especially if multiple variable sets could define the same key. If the environment can truly have only one match, consider adding a uniqueness constraint for clarity and maintainability.
| { | ||
| "name": "@ctrlplane/rule-engine", | ||
| "private": true, | ||
| "version": "0.1.0", | ||
| "type": "module", | ||
| "exports": { | ||
| ".": { | ||
| "types": "./src/index.ts", | ||
| "default": "./dist/index.js" | ||
| } | ||
| }, | ||
| "license": "MIT", | ||
| "scripts": { | ||
| "build": "tsc", | ||
| "dev": "tsc --watch", | ||
| "test": "vitest", | ||
| "clean": "rm -rf .turbo node_modules", | ||
| "format": "prettier --check . --ignore-path ../../.gitignore", | ||
| "lint": "eslint", | ||
| "typecheck": "tsc --noEmit --emitDeclarationOnly false" | ||
| }, | ||
| "dependencies": { | ||
| "@ctrlplane/db": "workspace:*", | ||
| "@ctrlplane/validators": "workspace:*", | ||
| "ts-is-present": "catalog:" | ||
| }, | ||
| "devDependencies": { | ||
| "@ctrlplane/eslint-config": "workspace:*", | ||
| "@ctrlplane/prettier-config": "workspace:*", | ||
| "@ctrlplane/tsconfig": "workspace:*", | ||
| "@types/node": "catalog:node22", | ||
| "eslint": "catalog:", | ||
| "prettier": "catalog:", | ||
| "typescript": "catalog:" | ||
| }, | ||
| "prettier": "@ctrlplane/prettier-config" | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Critical naming conflict with an existing package
The pipeline is failing because there's already a package named @ctrlplane/rule-engine at packages/rule-engine/package.json. This package needs to be renamed to something else, likely @ctrlplane/deployment-variables to match the directory name.
- "name": "@ctrlplane/rule-engine",
+ "name": "@ctrlplane/deployment-variables",🏁 Script executed:
#!/bin/bash
# Check if there's already a package at packages/rule-engine
if [ -f "packages/rule-engine/package.json" ]; then
echo "Verification: Found existing package at packages/rule-engine/package.json"
grep -n "\"name\":" packages/rule-engine/package.json
fi
# Check if the directory name matches other package patterns
echo "Current directory structure suggests the package should be named:"
basename "$(dirname "$(pwd)/packages/deployment-variables")"Length of output: 552
Critical Naming Conflict: Rename Package
The package in packages/deployment-variables/package.json is currently named "@ctrlplane/rule-engine", which conflicts with the existing package at packages/rule-engine/package.json that already uses that name. To resolve the pipeline failure and maintain naming consistency with the directory structure, please update the package name accordingly:
- File to update:
packages/deployment-variables/package.json - Action required: Change the package name from
"@ctrlplane/rule-engine"to"@ctrlplane/deployment-variables".
- "name": "@ctrlplane/rule-engine",
+ "name": "@ctrlplane/deployment-variables",📝 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.
| { | |
| "name": "@ctrlplane/rule-engine", | |
| "private": true, | |
| "version": "0.1.0", | |
| "type": "module", | |
| "exports": { | |
| ".": { | |
| "types": "./src/index.ts", | |
| "default": "./dist/index.js" | |
| } | |
| }, | |
| "license": "MIT", | |
| "scripts": { | |
| "build": "tsc", | |
| "dev": "tsc --watch", | |
| "test": "vitest", | |
| "clean": "rm -rf .turbo node_modules", | |
| "format": "prettier --check . --ignore-path ../../.gitignore", | |
| "lint": "eslint", | |
| "typecheck": "tsc --noEmit --emitDeclarationOnly false" | |
| }, | |
| "dependencies": { | |
| "@ctrlplane/db": "workspace:*", | |
| "@ctrlplane/validators": "workspace:*", | |
| "ts-is-present": "catalog:" | |
| }, | |
| "devDependencies": { | |
| "@ctrlplane/eslint-config": "workspace:*", | |
| "@ctrlplane/prettier-config": "workspace:*", | |
| "@ctrlplane/tsconfig": "workspace:*", | |
| "@types/node": "catalog:node22", | |
| "eslint": "catalog:", | |
| "prettier": "catalog:", | |
| "typescript": "catalog:" | |
| }, | |
| "prettier": "@ctrlplane/prettier-config" | |
| } | |
| { | |
| "name": "@ctrlplane/deployment-variables", | |
| "private": true, | |
| "version": "0.1.0", | |
| "type": "module", | |
| "exports": { | |
| ".": { | |
| "types": "./src/index.ts", | |
| "default": "./dist/index.js" | |
| } | |
| }, | |
| "license": "MIT", | |
| "scripts": { | |
| "build": "tsc", | |
| "dev": "tsc --watch", | |
| "test": "vitest", | |
| "clean": "rm -rf .turbo node_modules", | |
| "format": "prettier --check . --ignore-path ../../.gitignore", | |
| "lint": "eslint", | |
| "typecheck": "tsc --noEmit --emitDeclarationOnly false" | |
| }, | |
| "dependencies": { | |
| "@ctrlplane/db": "workspace:*", | |
| "@ctrlplane/validators": "workspace:*", | |
| "ts-is-present": "catalog:" | |
| }, | |
| "devDependencies": { | |
| "@ctrlplane/eslint-config": "workspace:*", | |
| "@ctrlplane/prettier-config": "workspace:*", | |
| "@ctrlplane/tsconfig": "workspace:*", | |
| "@types/node": "catalog:node22", | |
| "eslint": "catalog:", | |
| "prettier": "catalog:", | |
| "typescript": "catalog:" | |
| }, | |
| "prettier": "@ctrlplane/prettier-config" | |
| } |
🧰 Tools
🪛 GitHub Actions: CI
[error] 1-1: Failed to add workspace '@ctrlplane/rule-engine', it already exists at 'packages/rule-engine/package.json'.
Summary by CodeRabbit