fix: Create jobs for latest valid releases on env create #197
Conversation
WalkthroughThis pull request introduces several enhancements to the environment creation process within the web service. Key changes include the addition of a Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 6
🧹 Outside diff range and nitpick comments (9)
packages/db/drizzle/0030_easy_alex_wilder.sql (1)
1-1: Consider adding a comment to document the enum value's purpose.Adding inline documentation about when 'new_environment' trigger type is used would improve maintainability.
+-- Add value for triggering jobs when new environments are created ALTER TYPE "release_job_trigger_type" ADD VALUE 'new_environment';packages/job-dispatch/src/index.ts (1)
24-24: LGTM, but consider reordering exports for better organization.The new export follows the established patterns in the file. However, it might be better organized if grouped with other core functionality exports (before the utils export) rather than at the end of the file.
Consider moving this line before
export { isDateInTimeWindow } from "./utils.js";to maintain a clearer separation between core functionality and policy exports.apps/webservice/src/app/api/v1/environments/route.ts (1)
24-35: Consider adding error handling and input validation.While the function implementation is correct, consider these improvements:
- Add error handling for database insertion failures
- Validate the input array for empty/invalid entries before processing
Consider this enhanced implementation:
const createReleaseChannels = ( db: Tx, environmentId: string, releaseChannels: { channelId: string; deploymentId: string }[], -) => - db.insert(schema.environmentReleaseChannel).values( - releaseChannels.map(({ channelId, deploymentId }) => ({ - environmentId, - channelId, - deploymentId, - })), - ); +) => { + if (!releaseChannels.every(({ channelId, deploymentId }) => + channelId?.trim() && deploymentId?.trim())) { + throw new Error('Invalid release channel data: channelId and deploymentId are required'); + } + + return db.insert(schema.environmentReleaseChannel) + .values( + releaseChannels.map(({ channelId, deploymentId }) => ({ + environmentId, + channelId, + deploymentId, + })), + ) + .catch((error) => { + throw new Error(`Failed to create release channels: ${error.message}`); + }); +}packages/db/src/schema/deployment.ts (1)
61-66: LGTM: Well-structured relationship definitionThe
deploymentRelationsdefinition correctly establishes the one-to-one relationship between deployment and system tables, which is essential for maintaining referential integrity in the database schema.This relationship definition is particularly important for the environment creation process as it ensures:
- Type-safe access to related system data
- Proper referential integrity between deployments and systems
- Better query optimization possibilities through explicit relationship definitions
packages/db/src/schema/environment.ts (1)
54-70: Enhance the refinement function with a clear error message.The uniqueness check for
deploymentIdsis good, but could be more explicit with its error messaging to help API consumers understand validation failures.Consider updating the refinement:
.refine((channels) => { if (channels == null) return true; const deploymentIds = new Set(channels.map((c) => c.deploymentId)); return deploymentIds.size === channels.length; - }), + }, { + message: "Duplicate deploymentIds are not allowed in releaseChannels" + }),packages/job-dispatch/src/environment-creation.ts (2)
20-31: Optimize database query by minimizing fetched dataThe query in lines 20-31 fetches the entire environment along with all related data specified in the
withclause. If not all this data is necessary for the function, consider fetching only the required fields to improve performance and reduce database load.
13-96: Refactor function for enhanced readability and maintainabilityThe
createJobsForNewEnvironmentfunction handles multiple responsibilities, including data fetching, business logic, and dispatching jobs. Breaking it down into smaller, focused helper functions can improve readability, facilitate testing, and enhance maintainability.packages/node-sdk/src/schema.ts (2)
371-373: Ensure path parameters are specified for '{targetId}'In the
getTargetoperation, the path parametertargetIdshould be included in thepathsinterface for clarity and consistency.Consider adding
targetIdto the path parameters:// At line 62 - path?: never; + path: { + /** @description The target ID */ + targetId: string; + };
59-63: Specify path parameters for routes with dynamic segmentsFor routes like
"/v1/targets/{targetId}", it's important to define the path parameters to ensure correct typing and usage. Currently,pathis set tonever, which might lead to missing parameter definitions.Update the
parameterssection to includetargetId:// At lines 60-63 - parameters: { - query?: never; - header?: never; - path?: never; - cookie?: never; - }; + parameters: { + query?: never; + header?: never; + path: { + /** @description The target ID */ + targetId: string; + }; + cookie?: never; + };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
apps/webservice/src/app/api/v1/environments/route.ts(3 hunks)packages/db/drizzle/0030_easy_alex_wilder.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/deployment.ts(2 hunks)packages/db/src/schema/environment.ts(3 hunks)packages/db/src/schema/release.ts(1 hunks)packages/db/src/schema/system.ts(2 hunks)packages/job-dispatch/src/environment-creation.ts(1 hunks)packages/job-dispatch/src/index.ts(1 hunks)packages/node-sdk/src/schema.ts(1 hunks)
🔇 Additional comments (10)
packages/db/drizzle/0030_easy_alex_wilder.sql (1)
1-1: LGTM! Verify enum usage in constraints.
The ALTER TYPE statement is correctly formed and follows PostgreSQL best practices for enum modification.
Let's verify if this enum is used in any constraints or default values:
✅ Verification successful
Safe to proceed - no default values or constraints using the enum
Based on the search results, I can confirm that:
- The enum
release_job_trigger_typeis used as a column type withNOT NULLconstraint but has no default values - The enum values are consistently defined across migrations
- Adding a new enum value is a safe operation in PostgreSQL and won't affect existing data
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any constraints or default values using release_job_trigger_type
# Expected: Find any potential impact points that need attention
# Search for table definitions and constraints using this enum
rg -A 5 "release_job_trigger_type" packages/db/drizzle/
# Search for enum usage in TypeScript schema
rg -A 5 "release_job_trigger_type" packages/db/src/
Length of output: 31068
packages/db/src/schema/system.ts (2)
2-2: LGTM: Imports are properly organized and necessary.
The new imports are required for defining the relationships between system, environment, and deployment tables.
Also applies to: 7-8
51-54: LGTM: Relationships are properly defined.
The relationships between system, environments, and deployments are correctly established using drizzle-orm's relations API.
Let's verify the cascade delete behavior in related schemas:
✅ Verification successful
Cascade delete behavior is properly configured
The verification confirms that cascade delete is correctly implemented:
- Environment table has
onDelete: "cascade"for system references - Variable sets table has
onDelete: "cascade"for system references - Runbook table has
onDelete: "cascade"for system references - Deployment table uses a composite index with system ID, which ensures referential integrity
The cascade delete configuration ensures that when a system is deleted, all its associated environments, variable sets, runbooks, and deployments will be automatically removed, preventing orphaned records.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if cascade delete is properly handled in related tables
# Expected: Should find onDelete cascade configurations in environment and deployment schemas
# Search for system references in environment and deployment schemas
rg -A 5 "system.*references|references.*system" packages/db/src/schema/
Length of output: 3006
apps/webservice/src/app/api/v1/environments/route.ts (2)
2-2: LGTM: Import changes are appropriate.
The new imports support the added functionality for database transactions and job creation.
Also applies to: 10-10
69-69: Verify job creation implementation and impact.
The addition of automatic job creation for new environments is a significant change. Let's verify the implementation details.
✅ Verification successful
Job creation implementation is safe and well-structured
The implementation of createJobsForNewEnvironment is verified to be robust and safe:
- Only creates jobs when necessary (has targetFilter)
- Properly handles missing data cases with early returns
- Uses transactions for database operations
- Follows proper sequencing: creates triggers, approvals, and dispatches
- Has appropriate policy checks before dispatch
- Only used in the environment creation endpoint
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the job creation implementation and its usage
# Check the implementation of createJobsForNewEnvironment
echo "Checking createJobsForNewEnvironment implementation:"
rg -A 10 "export const createJobsForNewEnvironment"
# Check for any existing environment creation tests
echo "Checking for related tests:"
fd -e test.ts -e spec.ts -e test.js -e spec.js | xargs rg "createJobsForNewEnvironment"
Length of output: 1217
Script:
#!/bin/bash
# Let's get more context about the job creation implementation
# Get the complete implementation of createJobsForNewEnvironment
echo "Complete implementation of createJobsForNewEnvironment:"
rg -B 5 -A 30 "export const createJobsForNewEnvironment" "packages/job-dispatch/src/environment-creation.ts"
# Check for any imports and related functions in the same file
echo "File context and imports:"
cat "packages/job-dispatch/src/environment-creation.ts"
# Check for any tests in the job-dispatch package
echo "Tests in job-dispatch package:"
fd -e test.ts -e spec.ts . "packages/job-dispatch"
# Check where this function is being used
echo "Usage of createJobsForNewEnvironment:"
rg "createJobsForNewEnvironment"
Length of output: 5120
packages/db/src/schema/deployment.ts (1)
2-2: LGTM: Import statement is correctly placed
The addition of the relations import from 'drizzle-orm' is appropriate and follows the existing import organization pattern.
packages/db/drizzle/meta/_journal.json (1)
214-220: Verify migration sequence integrity.
The migration entry appears to be properly sequenced (idx: 30) and includes the necessary enum changes for release job triggers. However, let's verify the migration chain integrity.
✅ Verification successful
Migration sequence integrity verified successfully
The verification shows:
- Complete migration sequence from 0000 to 0030 with no gaps
- Proper chronological ordering (timestamps: 1730328775436 -> 1730603122173)
- Consistent version numbering (version: "7")
- Valid SQL migration adding new enum value 'new_environment' to release_job_trigger_type
- Proper breakpoint configuration maintained
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify migration sequence and related files
# Test 1: Check if migration file exists
echo "Checking for migration file..."
fd "0030_easy_alex_wilder.sql" packages/db/drizzle
# Test 2: Verify no gaps in migration sequence
echo "Verifying migration sequence..."
for i in {0..30}; do
idx=$(printf "%04d" $i)
fd "^${idx}_.*\.sql$" packages/db/drizzle
done
# Test 3: Check for the new enum value in the migration
echo "Checking enum value in migration..."
rg "new_environment.*release_job_trigger_type" packages/db/drizzle
Length of output: 4609
Script:
#!/bin/bash
# Let's check the content of the latest migration file and its relationship with the previous one
# Check the content of migration 0030
echo "Content of migration 0030:"
cat packages/db/drizzle/0030_easy_alex_wilder.sql
# Check the content of migration 0029 for context
echo -e "\nContent of migration 0029:"
cat packages/db/drizzle/0029_boring_frank_castle.sql
# Check the meta journal for these migrations
echo -e "\nMeta journal entries for migrations 29 and 30:"
rg -A 5 '"idx": 29,' packages/db/drizzle/meta/_journal.json
rg -A 5 '"idx": 30,' packages/db/drizzle/meta/_journal.json
Length of output: 1092
packages/db/src/schema/release.ts (1)
162-162: LGTM! Verify database migration and usage.
The addition of "new_environment" to the releaseJobTriggerType enum aligns with the PR objective of creating jobs for new environments. This is a backward-compatible change.
Let's verify the implementation:
✅ Verification successful
Let me check the migration file and the usage context to ensure proper implementation.
✅ Enum value properly integrated and used
The verification confirms:
- A proper database migration exists in
0030_easy_alex_wilder.sqlto add the new enum value - The enum is correctly used in
environment-creation.tsfor creating release job triggers during environment creation - The implementation aligns with the PR's objective
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the database migration and usage of the new enum value
# Test 1: Check if there's a corresponding migration file
echo "Checking for migration file..."
fd --type f "0.*\.sql$" "packages/db/drizzle" --exec grep -l "new_environment"
# Test 2: Check the usage of the new enum value
echo "Checking usage of new_environment..."
rg "new_environment" --type ts
Length of output: 435
Script:
#!/bin/bash
# Check the migration file content
echo "Migration file content:"
cat packages/db/drizzle/0030_easy_alex_wilder.sql
# Check the usage context in environment-creation.ts
echo -e "\nUsage in environment-creation.ts:"
rg -A 5 -B 5 "new_environment" packages/job-dispatch/src/environment-creation.ts
Length of output: 615
packages/db/src/schema/environment.ts (2)
82-85: LGTM! Well-structured system relation.
The one-to-one relationship between environment and system is properly defined with correct field references.
Line range hint 54-85: Verify database schema relationships for job creation flow.
The schema changes look good for supporting job creation during environment creation. Let's verify the table relationships to ensure they support this workflow.
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Database schema relationships properly support job creation flow
The verification confirms that:
- Environment has correct relationships with release channels and deployments
- Proper cascade delete behaviors are in place for dependent records
- No circular dependencies exist in the schema
- Foreign key references are properly defined with appropriate delete behaviors
The schema changes align perfectly with the job creation workflow requirements, maintaining referential integrity while allowing the creation of jobs for latest valid releases during environment creation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the database schema relationships needed for job creation
# Test 1: Check for circular dependencies in table relationships
echo "Checking for circular dependencies in table relationships..."
rg -A 5 "references\(\(\) =>"
# Test 2: Verify cascade delete behavior for related tables
echo "Verifying cascade delete behavior..."
rg "onDelete: \"cascade\""
# Test 3: Check for any orphaned foreign key references
echo "Checking for orphaned foreign key references..."
rg "references\(\(\) =>" | grep -v "onDelete"
Length of output: 35546
| .then(async (environment) => { | ||
| if ( | ||
| isPresent(ctx.body.releaseChannels) && | ||
| ctx.body.releaseChannels.length > 0 | ||
| ) | ||
| await createReleaseChannels( | ||
| ctx.db, | ||
| environment.id, | ||
| ctx.body.releaseChannels, | ||
| ); | ||
| await createJobsForNewEnvironment(ctx.db, environment); | ||
| return NextResponse.json({ environment }); | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling specificity and transaction rollback.
The error handling could be more specific to help diagnose issues:
- Different error messages for release channel creation vs job creation failures
- Ensure transaction rollback on failure
Consider this enhanced implementation:
.then(async (environment) => {
if (
isPresent(ctx.body.releaseChannels) &&
ctx.body.releaseChannels.length > 0
- )
+ ) {
await createReleaseChannels(
ctx.db,
environment.id,
ctx.body.releaseChannels,
);
+ }
await createJobsForNewEnvironment(ctx.db, environment);
return NextResponse.json({ environment });
})
- .catch(() =>
+ .catch((error) =>
NextResponse.json(
- { error: "Failed to create environment" },
+ {
+ error: "Failed to create environment",
+ details: error.message
+ },
{ status: 500 },
),
),📝 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.
| .then(async (environment) => { | |
| if ( | |
| isPresent(ctx.body.releaseChannels) && | |
| ctx.body.releaseChannels.length > 0 | |
| ) | |
| await createReleaseChannels( | |
| ctx.db, | |
| environment.id, | |
| ctx.body.releaseChannels, | |
| ); | |
| await createJobsForNewEnvironment(ctx.db, environment); | |
| return NextResponse.json({ environment }); | |
| }) | |
| .then(async (environment) => { | |
| if ( | |
| isPresent(ctx.body.releaseChannels) && | |
| ctx.body.releaseChannels.length > 0 | |
| ) { | |
| await createReleaseChannels( | |
| ctx.db, | |
| environment.id, | |
| ctx.body.releaseChannels, | |
| ); | |
| } | |
| await createJobsForNewEnvironment(ctx.db, environment); | |
| return NextResponse.json({ environment }); | |
| }) | |
| .catch((error) => | |
| NextResponse.json( | |
| { | |
| error: "Failed to create environment", | |
| details: error.message | |
| }, | |
| { status: 500 }, | |
| ), | |
| ), |
| }, | ||
| { | ||
| "idx": 30, | ||
| "version": "7", | ||
| "when": 1730603122173, | ||
| "tag": "0030_easy_alex_wilder", | ||
| "breakpoints": true |
There was a problem hiding this comment.
Fix the future timestamp in the migration entry.
The migration entry has a timestamp set to November 2024 (1730603122173), which is in the future. This could cause issues with migration ordering and execution in production environments.
Please update the timestamp to the current time when the migration was created.
| const releasePromises = deployments.map(async (deployment) => { | ||
| const envReleaseChannel = envReleaseChannels.find( | ||
| (erc) => erc.deploymentId === deployment.id, | ||
| ); | ||
| const policyReleaseChannel = policyReleaseChannels.find( | ||
| (prc) => prc.deploymentId === deployment.id, | ||
| ); | ||
| const { releaseFilter } = | ||
| envReleaseChannel?.releaseChannel ?? | ||
| policyReleaseChannel?.releaseChannel ?? | ||
| {}; | ||
|
|
||
| return db | ||
| .select() | ||
| .from(SCHEMA.release) | ||
| .where( | ||
| and( | ||
| eq(SCHEMA.release.deploymentId, deployment.id), | ||
| SCHEMA.releaseMatchesCondition(db, releaseFilter ?? undefined), | ||
| ), | ||
| ) | ||
| .orderBy(desc(SCHEMA.release.createdAt)) | ||
| .limit(1) | ||
| .then(takeFirstOrNull); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve performance by batching release retrieval
Currently, the code fetches the latest release for each deployment individually within a loop, resulting in multiple database queries. Consider modifying the code to retrieve the latest releases for all deployments in a single query. This can significantly improve performance by reducing the number of database round-trips.
| kind: string; | ||
| identifier: string; | ||
| version: string; | ||
| config: Record<string, never>; |
There was a problem hiding this comment.
Correct 'Record<string, never>' to 'Record<string, unknown>' for object properties
The use of Record<string, never> for properties like config, metadata, variables, jobAgentConfig, deployment, runbook, target, and environment implies that these objects cannot have any key-value pairs, as never indicates no possible value. To allow these properties to hold key-value pairs, consider changing the type to Record<string, unknown>.
Apply the following diff to fix the types:
// At line 264
- config: Record<string, never>;
+ config: Record<string, unknown>;
// At line 298
- metadata?: Record<string, never>;
+ metadata?: Record<string, unknown>;
// At lines 606-607
- metadata: Record<string, never>;
- config: Record<string, never>;
+ metadata: Record<string, unknown>;
+ config: Record<string, unknown>;
// At lines 629-630
- config: Record<string, never>;
- metadata: Record<string, never>;
+ config: Record<string, unknown>;
+ metadata: Record<string, unknown>;
// At line 637
- variables: Record<string, never>;
+ variables: Record<string, unknown>;
// At line 848
- jobAgentConfig: Record<string, never>;
+ jobAgentConfig: Record<string, unknown>;
// At line 927
- jobAgentConfig: Record<string, never>;
+ jobAgentConfig: Record<string, unknown>;
// At lines 930-934
- deployment?: Record<string, never>;
- config: Record<string, never>;
- runbook?: Record<string, never>;
- target?: Record<string, never>;
- environment?: Record<string, never>;
+ deployment?: Record<string, unknown>;
+ config: Record<string, unknown>;
+ runbook?: Record<string, unknown>;
+ target?: Record<string, unknown>;
+ environment?: Record<string, unknown>;Also applies to: 298-298, 606-607, 629-630, 637-637, 848-848, 927-927, 930-934
| }; | ||
| content: { | ||
| "application/json": { | ||
| sucess: boolean; |
There was a problem hiding this comment.
Fix typo in response property 'sucess'
There's a typo in the JSON response property; it should be success instead of sucess.
Apply this diff to correct the typo:
// At line 807
- sucess: boolean;
+ success: boolean;📝 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.
| sucess: boolean; | |
| success: boolean; |
| "application/json": { | ||
| job?: Record<string, never>; |
There was a problem hiding this comment.
Clarify type definition for 'job' property
The type Record<string, never> for the job property implies that it cannot hold any key-value pairs. If this property is intended to contain job details, consider defining a proper interface or using Record<string, unknown>.
Apply this diff:
// At line 876
- job?: Record<string, never>;
+ job?: Record<string, unknown>;📝 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.
| "application/json": { | |
| job?: Record<string, never>; | |
| "application/json": { | |
| job?: Record<string, unknown>; |
Summary by CodeRabbit
Release Notes
New Features
"new_environment"for categorizing job triggers related to new environments.Improvements
Bug Fixes
These changes enhance user experience by providing more robust environment management and clearer API interactions.