fix: Redeploy on var value change #211
Conversation
WalkthroughThe pull request introduces enhancements to the deployment variable management system, focusing on the 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: 0
🧹 Outside diff range and nitpick comments (4)
packages/job-dispatch/src/release-job-trigger.ts (1)
Line range hint
41-91: Consider documenting the general-purpose nature of deployment filteringWhile these changes were introduced to support redeployment on variable changes, the implementation provides a general-purpose deployment filtering capability that could be useful in other scenarios. Consider adding documentation to highlight this reusability aspect.
packages/api/src/router/deployment-variable.ts (3)
Line range hint
86-98: Handle potential null results from database query to prevent runtime errorsThe
takeFirstmethod may returnundefinedif no matching record is found. Destructuring properties from an undefined object will cause a runtime error. Ensure that you handle the case where the result isnullorundefined.Apply this diff to handle potential null values:
- const { deploymentId, defaultValueId } = await ctx.db + const result = await ctx.db .select() .from(deploymentVariableValue) .innerJoin( deploymentVariable, eq(deploymentVariableValue.variableId, deploymentVariable.id), ) .where(eq(deploymentVariableValue.id, input.id)) - .then(takeFirst) - .then((v) => ({ - deploymentId: v.deployment_variable.deploymentId, - defaultValueId: v.deployment_variable.defaultValueId, - })); + .then(takeFirstOrNull); + + if (!result) { + throw new Error(`Deployment variable value with id ${input.id} not found`); + } + + const { deploymentId, defaultValueId } = { + deploymentId: result.deployment_variable.deploymentId, + defaultValueId: result.deployment_variable.defaultValueId, + };
Line range hint
100-126: EnsureupdatedValueis not null before proceedingAfter updating the deployment variable value,
takeFirstmay returnundefinedif the update did not affect any rows. Attempting to useupdatedValuewhen it isundefinedcould lead to runtime errors.Apply this diff to handle potential null
updatedValue:const updatedValue = await ctx.db.transaction((tx) => tx .update(deploymentVariableValue) .set(input.data) .where(eq(deploymentVariableValue.id, input.id)) .returning() - .then(takeFirst) + .then(takeFirstOrNull) .then(async (updatedValue) => { + if (!updatedValue) { + throw new Error(`Failed to update deployment variable value with id ${input.id}`); + } + if (input.data.default && defaultValueId !== updatedValue.id) await tx .update(deploymentVariable) .set({ defaultValueId: updatedValue.id }) .where(eq(deploymentVariable.id, updatedValue.variableId)); if ( input.data.default === false && defaultValueId === updatedValue.id ) await tx .update(deploymentVariable) .set({ defaultValueId: null }) .where(eq(deploymentVariable.id, updatedValue.variableId)); return updatedValue; }), );
127-148: Refactor chained promises toasync/awaitfor improved readabilityThe current use of nested
.then()callbacks can make the code difficult to read and maintain. Refactoring the code to useasync/awaitwill enhance clarity and make error handling more straightforward.Apply this refactor:
if (input.data.value != null) { - await ctx.db.query.target - .findMany({ - where: targetMatchesMetadata(ctx.db, updatedValue.targetFilter), - }) - .then((targets) => - createReleaseJobTriggers(ctx.db, "variable_changed") - .causedById(ctx.session.user.id) - .targets(targets.map((t) => t.id)) - .deployments([deploymentId]) - .filter(isPassingNoPendingJobsPolicy) - .filter(isPassingReleaseStringCheckPolicy) - .insert() - .then((triggers) => - dispatchReleaseJobTriggers(ctx.db) - .releaseTriggers(triggers) - .filter(isPassingAllPolicies) - .then(cancelOldReleaseJobTriggersOnJobDispatch) - .dispatch(), - ), - ); + const targets = await ctx.db.query.target.findMany({ + where: targetMatchesMetadata(ctx.db, updatedValue.targetFilter), + }); + + const triggers = await createReleaseJobTriggers(ctx.db, "variable_changed") + .causedById(ctx.session.user.id) + .targets(targets.map((t) => t.id)) + .deployments([deploymentId]) + .filter(isPassingNoPendingJobsPolicy) + .filter(isPassingReleaseStringCheckPolicy) + .insert(); + + await dispatchReleaseJobTriggers(ctx.db) + .releaseTriggers(triggers) + .filter(isPassingAllPolicies) + .then(cancelOldReleaseJobTriggersOnJobDispatch) + .dispatch(); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
packages/api/src/router/deployment-variable.ts(4 hunks)packages/db/drizzle/0034_brave_rogue.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/release.ts(1 hunks)packages/job-dispatch/src/release-job-trigger.ts(3 hunks)
🔇 Additional comments (9)
packages/db/drizzle/0034_brave_rogue.sql (2)
1-1: LGTM! Safe enum extension.
The ALTER TYPE statement correctly adds the new enum value. This is a safe operation that won't impact existing data or require table locks.
1-1: Verify corresponding TypeScript enum updates.
Let's ensure the TypeScript schema is properly synchronized with this enum change.
✅ Verification successful
TypeScript enum is properly synchronized with the new value
The TypeScript enum definition in packages/db/src/schema/release.ts already includes the variable_changed value, matching the SQL migration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the corresponding TypeScript enum includes the new value
# and check for any constraints using this enum type
# Check TypeScript enum definition
echo "Checking TypeScript enum definition:"
rg -A 10 "releaseJobTriggerType" packages/db/src/schema/
# Check for any constraints or checks using this enum
echo "Checking for constraints using this enum:"
rg -l "release_job_trigger_type" packages/db/drizzle/ | grep -v "0034_brave_rogue.sql"
Length of output: 3476
packages/db/drizzle/meta/_journal.json (1)
242-248: LGTM! Verify the corresponding migration file.
The new journal entry follows the correct format and sequential order.
Let's verify the corresponding migration file exists and contains the expected enum changes:
✅ Verification successful
Migration file verified and contains the expected enum changes ✅
The migration file 0034_brave_rogue.sql exists and correctly adds the new 'variable_changed' value to the release_job_trigger_type enum, which aligns with the journal entry and the intended changes for the deployment variable management system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the migration file exists and contains the enum changes
# Check if the migration file exists and contains the enum changes
fd -t f "0034_brave_rogue.sql" packages/db/drizzle/ -x cat {} | grep -A 5 "release_job_trigger_type"
Length of output: 168
packages/job-dispatch/src/release-job-trigger.ts (3)
41-41: LGTM: Property follows established patterns
The new deploymentIds property is well-structured and consistent with existing filter properties.
77-80: LGTM: Method follows builder pattern correctly
The deployments method is well-implemented, maintaining consistency with other filter methods and proper method chaining.
91-91: Verify query behavior with deployment filtering
The deployment filter implementation looks correct, but let's verify that it works as expected with different combinations of filters.
Let's analyze the query construction:
✅ Verification successful
Let me analyze the query construction further by checking the base query and other deployment-related filters.
The deployment filter implementation is correctly integrated
The implementation is safe and consistent with the codebase's query construction pattern because:
- The deployment table is properly joined in
_baseQuery()viainnerJoin(deployment, eq(deployment.systemId, environment.systemId)) - The new filter follows the same pattern as other filters in the
_where()method - No conflicting deployment filters exist in the query chain
- The condition is properly guarded with
this.deploymentIds &&to handle undefined cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential conflicts or edge cases in deployment filtering
# Look for other places where deployment filtering might be applied
rg -l 'deployment.*id' --type ts
# Check for any existing deployment-related queries that might conflict
ast-grep --pattern 'inArray(deployment.id, $_)'
Length of output: 3853
Script:
#!/bin/bash
# Check the base query and other deployment-related filters in release-job-trigger.ts
rg -A 10 "_baseQuery" packages/job-dispatch/src/release-job-trigger.ts
# Look for other deployment filters in the same file
rg "deployment\." -A 3 packages/job-dispatch/src/release-job-trigger.ts
Length of output: 1858
packages/db/src/schema/release.ts (1)
169-169: LGTM! Verify database schema migration.
The new enum value variable_changed follows the existing pattern of change-based triggers and aligns with the PR objective.
Let's verify that this enum value is properly added to the database schema:
✅ Verification successful
Migration verified - enum value properly added to schema
The database schema migration is properly handled:
- Migration file
0034_brave_rogue.sqladds thevariable_changedvalue to therelease_job_trigger_typeenum - The enum value is also reflected in the corresponding snapshot file
meta/0034_snapshot.json
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the enum value is added to the database schema migration
# Expected: Find a migration file that adds 'variable_changed' to the release_job_trigger_type enum
# Look for migration files that modify the release_job_trigger_type enum
fd --type f . "packages/db/drizzle" --exec rg -l "release_job_trigger_type"
# Look for the specific enum value addition
rg -A 5 -B 5 "variable_changed" "packages/db/drizzle"
Length of output: 2626
packages/api/src/router/deployment-variable.ts (2)
19-26: Imports are appropriate and correctly utilized
All imported functions from @ctrlplane/job-dispatch are used appropriately within the code. This enhances the deployment variable management functionality as intended.
127-148: Verify that all policies are correctly applied to release job triggers
Ensure that the policies are applied in the correct order and that no necessary policy checks are missing before dispatching the release job triggers.
Run the following script to check the policy application in release-job-trigger.ts:
This script searches for all instances where policies are applied in the release-job-trigger module to confirm that they are properly implemented.
✅ Verification successful
The file path in the previous attempt might have been incorrect. Let's search broadly across the codebase to find where these policies are defined and implemented.
Policies are correctly applied in the expected order
Based on the codebase analysis, the policies in the code snippet are correctly implemented and applied in a proper sequence:
isPassingNoPendingJobsPolicy- Ensures no pending jobs existisPassingReleaseStringCheckPolicy- Validates release string formatisPassingAllPolicies- Applies all remaining policies in the correct order:- Locking policy
- Approval policy
- Criteria policy
- Concurrency policy
- Release dependency policy
- Job rollout policy
- No active jobs policy
- Newer than last active policy
- Release window policy
The implementation in deployment-variable.ts follows the standard pattern used across the codebase for policy application, as seen in other files like release.ts, target.ts, and release-deploy.ts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all necessary policies are applied to release job triggers.
# Test: Search for policy applications in 'release-job-trigger.ts'. Expect: All policies are applied.
rg 'isPassing.*Policy' packages/job-dispatch/src/release-job-trigger.ts
Length of output: 73
Script:
#!/bin/bash
# Let's try searching in the entire codebase for policy-related functions
rg -A 2 'isPassing.*Policy|isPassingAllPolicies'
# Also search for the specific policy functions mentioned in the code
rg -A 2 'isPassingNoPendingJobsPolicy|isPassingReleaseStringCheckPolicy'
# Search for any policy-related type definitions or interfaces
ast-grep --pattern 'interface $_Policy {
$$$
}'
Length of output: 19485
Summary by CodeRabbit
Release Notes
New Features
variable_changedfor improved trigger conditions in release jobs.Bug Fixes
Documentation