feat: Enhance environment validation and configuration for Temporal worker #33
Conversation
|
Warning Rate limit exceeded@anatolyshipitz has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 15 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 Walkthrough## Walkthrough
This change introduces environment variable validation for a Redmine database connection in the `workers/main` project. It adds a new configuration schema and utility function for validation, updates test and runtime configurations to use `.env.test`, and adjusts dependencies and test coverage settings. The worker configuration is also simplified.
## Changes
| File(s) | Change Summary |
|----------------------------------------------|--------------------------------------------------------------------------------------------------------|
| workers/main/.env.test | Added test environment file defining Redmine DB and DEBUG variables. |
| workers/main/package.json | Added devDependencies: `@temporalio/testing`, `dotenv`, `source-map-support`, `uuid`; reordered some dependencies. |
| workers/main/src/__tests__/utils.test.ts | Added tests for the new `validateEnv` utility function. |
| workers/main/src/common/utils.ts | Added `validateEnv` function to check environment variable schema validation and handle errors. |
| workers/main/src/configs/index.ts | Integrated `redmineDatabaseSchema` into the environment validation schema. |
| workers/main/src/configs/redmineDatabase.ts | Added Redmine DB config object and validation schema using Zod. |
| workers/main/src/configs/worker.ts | Removed `workflowsPath` property and related `path` import from worker config. |
| workers/main/vitest.config.ts | Loads `.env.test` before running tests; removed code coverage thresholds. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Process
participant EnvValidator (validateEnv)
participant ConfigSchema
Process->>EnvValidator: Call validateEnv()
EnvValidator->>ConfigSchema: Check validationResult
alt Validation success
EnvValidator-->>Process: Continue execution
else Validation failure
EnvValidator->>Process: Log error(s), exit with code 1
endPossibly related PRs
Suggested reviewers
|
anatolyshipitz
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
workers/main/src/common/utils.ts (1)
8-8: Consider improving path formatting for nested variables.The current
path.join('.')works for simple cases, but for nested environment variable validation, you might want to consider a more robust path formatting approach.Consider this alternative for better path handling:
- `Missing or invalid environment variable: ${path.join('.') || '(unknown variable)'} (${errorMessage})`, + `Missing or invalid environment variable: ${path.length > 0 ? path.join('.') : '(root level)'} (${errorMessage})`,workers/main/src/configs/redmineDatabase.ts (1)
10-15: Consider enhancing schema validation for database configuration.While the current schema validates that environment variables are strings, it doesn't protect against empty strings or provide meaningful validation for database connection parameters.
Consider adding more robust validation:
export const redmineDatabaseSchema = z.object({ - REDMINE_DB_HOST: z.string(), - REDMINE_DB_USER: z.string(), - REDMINE_DB_PASSWORD: z.string(), - REDMINE_DB_NAME: z.string(), + REDMINE_DB_HOST: z.string().min(1, 'Database host cannot be empty'), + REDMINE_DB_USER: z.string().min(1, 'Database user cannot be empty'), + REDMINE_DB_PASSWORD: z.string().min(1, 'Database password cannot be empty'), + REDMINE_DB_NAME: z.string().min(1, 'Database name cannot be empty'), });workers/main/src/__tests__/utils.test.ts (2)
3-5: Simplify the mock import path.The mock path contains redundant directory navigation that could be simplified for better readability.
-vi.mock('../common/../configs', () => ({ +vi.mock('../configs', () => ({ validationResult: { success: true }, }));
7-7: Update import path to match the simplified mock path.For consistency with the mock path simplification above.
-import * as configs from '../common/../configs'; +import * as configs from '../configs';workers/main/src/configs/worker.ts (1)
4-6: Consider making the task queue configurable.The task queue is currently hardcoded as 'main-queue'. Consider making this configurable through environment variables to support different environments (dev, staging, prod).
export const workerConfig: WorkerOptions = { - taskQueue: 'main-queue', + taskQueue: process.env.WORKER_TASK_QUEUE || 'main-queue', };You would also need to add this to the schema:
export const workerSchema = z.object({ WORKFLOWS_PATH: z.string().optional(), + WORKER_TASK_QUEUE: z.string().default('main-queue'), });workers/main/package.json (1)
24-25: Inconsistent version pinning strategy.Notice the inconsistency:
source-map-supportuses^0.5.21whilets-nodeis pinned to10.9.1(caret removed). Consider establishing a consistent versioning strategy for better build reproducibility.For better build reproducibility, consider either:
- Pin all versions (remove carets) for exact reproducibility
- Use carets consistently for patch updates
- "source-map-support": "^0.5.21", + "source-map-support": "0.5.21",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
workers/main/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
workers/main/.env.test(1 hunks)workers/main/eslint.config.mjs(1 hunks)workers/main/package.json(1 hunks)workers/main/src/__tests__/utils.test.ts(1 hunks)workers/main/src/common/utils.ts(1 hunks)workers/main/src/configs/index.ts(1 hunks)workers/main/src/configs/redmineDatabase.ts(1 hunks)workers/main/src/configs/temporal.ts(1 hunks)workers/main/src/configs/worker.ts(1 hunks)workers/main/src/index.ts(1 hunks)workers/main/vitest.config.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
workers/main/src/index.ts (1)
workers/main/src/common/utils.ts (1)
validateEnv(3-15)
workers/main/src/configs/index.ts (3)
workers/main/src/configs/temporal.ts (1)
temporalSchema(10-12)workers/main/src/configs/worker.ts (1)
workerSchema(8-10)workers/main/src/configs/redmineDatabase.ts (1)
redmineDatabaseSchema(10-15)
workers/main/src/common/utils.ts (1)
workers/main/src/configs/index.ts (1)
validationResult(5-8)
workers/main/src/__tests__/utils.test.ts (1)
workers/main/src/common/utils.ts (1)
validateEnv(3-15)
🔇 Additional comments (13)
workers/main/.env.test (1)
1-5: LGTM! Well-structured test environment configuration.The environment variables are appropriately configured for testing with safe, non-production values. The naming conventions follow standard practices, and the inclusion of an optional DEBUG variable provides flexibility for development.
workers/main/vitest.config.ts (2)
1-1: Good addition of dotenv for test environment loading.The import sets up environment variable loading for tests.
4-4: Proper placement of environment variable loading.Loading the
.env.testfile before defining the Vitest configuration ensures test environment variables are available during setup.workers/main/eslint.config.mjs (2)
45-45: Appropriate relaxation of the require-await rule.Disabling this rule provides flexibility for async functions that may not need explicit await statements, which aligns with the new validation patterns in the codebase.
46-46: Good exception for console.error in logging rules.Allowing
console.errorwhile restricting other console methods strikes the right balance for error logging in the validation utility while maintaining clean code practices.workers/main/src/common/utils.ts (1)
12-14: Appropriate error handling and process termination.Using
console.errorfor logging andprocess.exit(1)for termination is correct for environment validation failures, as the application cannot proceed safely without proper configuration.workers/main/src/index.ts (1)
1-4: Excellent implementation of early environment validation.The placement of environment validation at the top level ensures that any environment issues are caught before the application logic runs. This follows the fail-fast principle and prevents runtime errors due to misconfiguration.
workers/main/src/configs/index.ts (1)
5-8: Well-structured schema composition and validation.The sequential merging of schemas followed by
safeParseis an excellent approach that:
- Combines all environment validation rules in one place
- Uses safe parsing to avoid throwing exceptions during validation
- Provides detailed error information through the validation result
This creates a robust foundation for environment validation across the application.
workers/main/src/__tests__/utils.test.ts (1)
18-58: Excellent test coverage for environment validation.The test suite comprehensively covers both success and failure scenarios with proper mocking and verification of side effects. The use of spies to verify console.error and process.exit calls ensures the function behaves correctly in all cases.
workers/main/src/configs/worker.ts (1)
8-10:❓ Verification inconclusive
Verify the usage pattern for WORKFLOWS_PATH.
The
WORKFLOWS_PATHis defined as optional in the schema. Please ensure this aligns with how it's used in the application, particularly in the worker initialization.
🏁 Script executed:
#!/bin/bash # Description: Check how WORKFLOWS_PATH is used in the codebase # Expected: Find usage patterns to confirm if optional is appropriate rg -A 3 "WORKFLOWS_PATH" --type tsLength of output: 149
🏁 Script executed:
#!/bin/bash # Locate where workerSchema is used rg "workerSchema" -n # Find imports of the worker config schema rg "configs/worker" -n # Confirm all occurrences of WORKFLOWS_PATH in codebase rg "WORKFLOWS_PATH" -nLength of output: 356
🏁 Script executed:
#!/bin/bash # Display the config schema and loading logic sed -n '1,200p' workers/main/src/configs/index.tsLength of output: 324
Double-check WORKFLOWS_PATH usage
I searched the repository and only found
WORKFLOWS_PATHin your Zod schema—no runtime code references it. Please verify that:
- It’s intentionally optional and you have a fallback/default path in your worker initialization.
- If it’s actually required for correct operation, you should update the schema to make it a required string (
z.string()) or provide a default value in code.Files to review:
- workers/main/src/configs/worker.ts
workers/main/src/configs/temporal.ts (1)
1-12: Well-implemented Temporal configuration.The configuration follows good practices:
- Clear separation of concerns with the default constant
- Proper environment variable fallback
- Consistent schema validation with defaults
- Type safety with NativeConnectionOptions
The default address
temporal:7233suggests containerized deployment, which is appropriate for most modern setups.workers/main/package.json (2)
32-39:✅ Verification successful
Verify package versions for security vulnerabilities.
Please ensure all newly added dependencies are free from known security vulnerabilities, especially the production dependencies.
🏁 Script executed:
#!/bin/bash # Description: Check for security advisories for the new dependencies # Expected: No critical vulnerabilities in the specified versions packages=("@temporalio/[email protected]" "@temporalio/[email protected]" "@temporalio/[email protected]" "@temporalio/[email protected]" "[email protected]" "[email protected]") for package in "${packages[@]}"; do echo "Checking $package..." gh api graphql -f query=" { securityVulnerabilities(first: 5, ecosystem: NPM, package: \"${package%@*}\") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }" doneLength of output: 4345
All specified dependencies are free from known security vulnerabilities
I’ve verified the versions in workers/main/package.json against GitHub’s advisories:
- No advisories for @temporalio/activity, @temporalio/client, @temporalio/worker or @temporalio/workflow at v1.11.8.
- mysql2 had HIGH/CRITICAL issues in versions < 3.9.8, but v3.14.1 is beyond the patched range.
- Zod had a DoS vulnerability in ≤ 3.22.2, but v3.25.17 is patched.
No further action required.
13-13:❓ Verification inconclusive
Verify compatibility with the major @types/node upgrade.
The upgrade from
@types/node20.17.48 to 22.15.21 is a major version change that could introduce breaking changes. Ensure all TypeScript code is compatible with Node.js 22 type definitions.
🏁 Script executed:
#!/bin/bash # Description: Check for potential Node.js compatibility issues # Expected: No TypeScript compilation errors # Check for any TypeScript compilation issues find . -name "*.ts" -not -path "./node_modules/*" | head -10 | xargs -I {} echo "Checking: {}" # Look for Node.js version requirements in other config files rg -A 2 -B 2 "node.*version|engines" --type jsonLength of output: 66115
Confirm TypeScript compatibility with @types/node v22
The previous script only listed your
.tsfiles—it didn’t catch any type‐checking errors. Please run a full compile check to surface breaking changes from bumping@types/node20.x→22.15.21:npm ci npx tsc --noEmitIf you see errors, either adjust the code or reconsider the type upgrade. Also, ensure your
package.json:
- Has an
engines.nodefield matching your supported Node runtime (e.g."node": ">=18").- Uses a consistent version-pinning strategy (we recommend
^semver prefixes across deps).Finally, don’t forget to perform an
npm auditto verify the security of all upgraded packages.
This commit removes the unused `workflowsPath` field from the worker configuration, simplifying the setup. Additionally, it introduces an environment validation utility to verify required environment variables, improving runtime robustness. Finally, updates to dependencies and `package-lock.json` reflect added packages like `uuid` and `dotenv` for functionality and testing.
anatolyshipitz
force-pushed
the
feature/64207_redmine_config
branch
from
231c918 to
c47d37f
Compare
🔍 Vulnerabilities of
|
| digest | sha256:47a7df350323c6da2c7584fdcf377d6898c6b1c7569cf2cfde0d40d89f25e801 |
| vulnerabilities |     |
| platform | linux/amd64 |
| size | 243 MB |
| packages | 1628 |
📦 Base Image node:20-alpine
| also known as |
|
| digest | sha256:37a5a350292926f98d48de9af160b0a3f7fcb141566117ee452742739500a5bd |
| vulnerabilities |   |
Description
| ||||||||||||
Description
| ||||||||||||
Description
| ||||||||||||
Description
| ||||||||||||
Description
| ||||||||||||
Description
| ||||||||||||
Description
|
This commit updates the variable name from `message` to `errorMessage` in the `validateEnv` function within the `utils.ts` file. This change improves clarity and consistency in error logging for missing or invalid environment variables, ensuring that the correct variable is referenced when logging errors. The overall functionality of the environment validation remains unchanged.
|
.env.testfile for environment variable configuration.validateEnvfunction to ensure required environment variables are set, logging errors and exiting if validation fails.package.jsonandpackage-lock.jsonto include new dependencies for environment validation and database configuration.validateEnvfunction to ensure proper functionality.These changes improve the robustness of the Temporal worker by enforcing environment variable validation and enhancing configuration management.