fix: Deploy release if it is after latest non-pending #200
Conversation
WalkthroughThe pull request introduces significant modifications to the release deployment and job dispatching logic within the application. It renames the function 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 (1)
packages/job-dispatch/src/policy-checker.ts (1)
41-43: Consider refactoring to reduce code duplicationThe new function largely duplicates code from
isPassingAllPolicies. Consider refactoring to extract common logic while maintaining clarity.Here's a suggested refactor:
+const BASE_POLICY_CHECKS: ReleaseIdPolicyChecker[] = [ + isPassingLockingPolicy, + isPassingApprovalPolicy, + isPassingCriteriaPolicy, + isPassingConcurrencyPolicy, + isPassingReleaseDependencyPolicy, + isPassingJobRolloutPolicy, + isPassingNoActiveJobsPolicy, + isPassingReleaseWindowPolicy, +]; + +const applyPolicyChecks = async ( + db: Tx, + releaseJobTriggers: schema.ReleaseJobTrigger[], + checks: ReleaseIdPolicyChecker[] +) => { + if (releaseJobTriggers.length === 0) return []; + let passingJobs = releaseJobTriggers; + for (const check of checks) passingJobs = await check(db, passingJobs); + return passingJobs; +}; + export const isPassingAllPolicies = async ( db: Tx, releaseJobTriggers: schema.ReleaseJobTrigger[], ) => { - if (releaseJobTriggers.length === 0) return []; - const checks: ReleaseIdPolicyChecker[] = [ - isPassingLockingPolicy, - isPassingApprovalPolicy, - isPassingCriteriaPolicy, - isPassingConcurrencyPolicy, - isPassingReleaseDependencyPolicy, - isPassingJobRolloutPolicy, - isPassingNoActiveJobsPolicy, - isPassingNewerThanLastActiveReleasePolicy, - isPassingReleaseWindowPolicy, - ]; - - let passingJobs = releaseJobTriggers; - for (const check of checks) passingJobs = await check(db, passingJobs); - - return passingJobs; + return applyPolicyChecks(db, releaseJobTriggers, [ + ...BASE_POLICY_CHECKS, + isPassingNewerThanLastActiveReleasePolicy, + ]); }; export const isPassingAllPoliciesExceptNewerThanLastActive = async ( db: Tx, releaseJobTriggers: schema.ReleaseJobTrigger[], ) => { - if (releaseJobTriggers.length === 0) return []; - const checks: ReleaseIdPolicyChecker[] = [ - isPassingLockingPolicy, - isPassingApprovalPolicy, - isPassingCriteriaPolicy, - isPassingConcurrencyPolicy, - isPassingReleaseDependencyPolicy, - isPassingJobRolloutPolicy, - isPassingNoActiveJobsPolicy, - isPassingReleaseWindowPolicy, - ]; - - let passingJobs = releaseJobTriggers; - for (const check of checks) passingJobs = await check(db, passingJobs); - - return passingJobs; + return applyPolicyChecks(db, releaseJobTriggers, BASE_POLICY_CHECKS); };This refactor:
- Extracts common policy checks into a constant
- Creates a reusable function for applying checks
- Reduces duplication while maintaining clear intent
- Makes it easier to maintain the list of base policies
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
packages/api/src/router/release-deploy.ts(3 hunks)packages/job-dispatch/src/policies/release-sequencing.ts(2 hunks)packages/job-dispatch/src/policy-checker.ts(1 hunks)packages/job-dispatch/src/release-sequencing.ts(3 hunks)
🔇 Additional comments (11)
packages/job-dispatch/src/policy-checker.ts (2)
11-14: LGTM: Import statements are properly structured
The new imports are correctly added and follow the established conventions.
21-39: Verify the order of policy checks
The order of policy checks is crucial as they are applied sequentially. The new isPassingNewerThanLastActiveReleasePolicy is placed near the end of the chain, just before isPassingReleaseWindowPolicy. Please verify if this ordering is intentional and optimal for performance.
Consider:
- More restrictive checks should be placed earlier to fail fast
- Computationally expensive checks should be placed later
✅ Verification successful
Based on the code analysis, I can now generate the final response since I have enough information about the policy checks' implementations.
The policy check order is optimal and intentional
The current order follows good practices for performance and reliability:
isPassingLockingPolicy- Fast DB check, should be first as it's a fundamental blocking conditionisPassingApprovalPolicy- Simple approval status check, lightweightisPassingCriteriaPolicy- Success rate validation, relatively simpleisPassingConcurrencyPolicy- Active jobs check, more complex but essential early filterisPassingReleaseDependencyPolicy- Complex dependency resolution, correctly placed mid-chainisPassingJobRolloutPolicy- Gradual rollout logic, depends on previous checksisPassingNoActiveJobsPolicy- Complex job state check, appropriately lateisPassingNewerThanLastActiveReleasePolicy- Correct placement as it requires active release contextisPassingReleaseWindowPolicy- Time-based check, appropriately last as it's independent
The placement of isPassingNewerThanLastActiveReleasePolicy before the window policy is correct as it:
- Depends on active release information gathered by previous checks
- Is more restrictive than the window policy
- Has moderate complexity (single subquery + release fetch)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Analyze the complexity and dependencies of each policy check
# Search for the implementation of each policy check to understand their complexity
echo "Analyzing policy implementations..."
for policy in "isPassingLockingPolicy" "isPassingApprovalPolicy" "isPassingCriteriaPolicy" "isPassingConcurrencyPolicy" "isPassingReleaseDependencyPolicy" "isPassingJobRolloutPolicy" "isPassingNoActiveJobsPolicy" "isPassingNewerThanLastActiveReleasePolicy" "isPassingReleaseWindowPolicy"; do
echo "\nAnalyzing $policy:"
rg -A 10 "export const $policy" --type ts
done
Length of output: 12501
packages/job-dispatch/src/release-sequencing.ts (3)
3-3: LGTM: Import changes are correct.
The addition of isNull import is properly placed and utilized in the SQL query.
43-43: Consider adding an index for the LEFT JOIN.
The change from INNER JOIN to LEFT JOIN could impact query performance. Consider adding an index on environment.policyId if not already present.
52-55:
Replace magic number with explicit enum value.
Using .at(1) to access enum values is fragile and could break if the enum order changes. Consider using the explicit enum value for better maintainability.
Additionally, please add a comment explaining the business logic behind these conditions:
- Why we're checking for this specific enum value
- Why null values are now acceptable
✅ Verification successful
From the search results, I found the definition of releaseSequencingType enum in packages/db/src/schema/environment.ts:
export const releaseSequencingType = pgEnum("release_sequencing_type", [
"wait",
"cancel",
]);Now I can generate the final response:
Replace .at(1) with explicit 'cancel' enum value
The magic number .at(1) refers to the 'cancel' value in the releaseSequencingType enum. For better maintainability and to avoid potential issues if the enum order changes, use the explicit value:
${schema.environmentPolicy.releaseSequencing} = 'cancel'This condition is used to check if the environment policy is configured to cancel existing releases when a new one is triggered, as opposed to waiting for them to complete ('wait' value).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find the enum definition to suggest the explicit value
# Search for releaseSequencingType enum definition
ast-grep --pattern 'enum releaseSequencingType {
$$$
}'
Length of output: 59
Script:
#!/bin/bash
# Let's try different approaches to find the enum definition
# Search for files containing "releaseSequencingType"
fd -t f | xargs rg "releaseSequencingType" -A 5 -B 5
# Also search for schema definitions that might contain this enum
rg "schema\." -A 5 -B 5
Length of output: 186302
packages/api/src/router/release-deploy.ts (3)
19-19: LGTM: Import of new policy check function.
The addition of isPassingAllPoliciesExceptNewerThanLastActive aligns with the PR objective of fixing deployment of releases after the latest non-pending release.
165-167: LGTM: Consistent policy checks across deployment scenarios.
The policy checks in toTarget mirror those in toEnvironment, maintaining consistent behavior whether deploying to an environment or a specific target.
94-96: LGTM: Improved policy checks for forced vs non-forced releases.
The conditional policy checks are well-structured:
- Forced releases only verify locking policy, appropriate for override scenarios
- Non-forced releases maintain all policy checks except the "newer than last active" check, allowing deployment of releases after the latest non-pending while maintaining other safety checks
Let's verify the policy checker implementation:
✅ Verification successful
Policy implementation verified and confirmed correct
The implementation is well-structured and matches the intended behavior:
-
isPassingAllPoliciesExceptNewerThanLastActiveinpackages/job-dispatch/src/policy-checker.tsapplies all essential policies:- Locking policy
- Approval policy
- Criteria policy
- Concurrency policy
- Release dependency policy
- Job rollout policy
- No active jobs policy
- Release window policy
-
isPassingLockingPolicyinpackages/job-dispatch/src/lock-checker.tsis correctly used as the only check for forced releases
The conditional policy application in the review code correctly implements the intended behavior, where forced releases bypass most checks while maintaining essential safety through the locking policy.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the policy checker implementation
# Expected: Find the policy checker function and its tests to ensure correct behavior
# Search for the policy checker implementation
echo "Searching for policy checker implementation..."
rg -t typescript "isPassingAllPoliciesExceptNewerThanLastActive" -A 10
# Search for related tests
echo "Searching for related tests..."
rg -t typescript "describe.*isPassingAllPoliciesExceptNewerThanLastActive" -A 10
Length of output: 397
Script:
#!/bin/bash
# Let's try without typescript file type flag since it's not recognized
# Search for the policy checker implementation
echo "Searching for policy checker implementation..."
rg "isPassingAllPoliciesExceptNewerThanLastActive" -A 10
# Search for related tests
echo "Searching for related tests..."
rg "describe.*isPassingAllPoliciesExceptNewerThanLastActive" -A 10
# Also search for the locking policy implementation for completeness
echo "Searching for locking policy implementation..."
rg "isPassingLockingPolicy" -A 10
Length of output: 10397
packages/job-dispatch/src/policies/release-sequencing.ts (3)
1-8: Imports are correctly updated to support new functionalities
The added imports are necessary for the new functions and ensure all dependencies are properly included.
72-93: latestActiveReleaseSubQuery function is well-implemented
The subquery correctly constructs a query to retrieve the latest active releases, partitioned by deployment and environment, with appropriate ranking.
101-159: isPassingNewerThanLastActiveReleasePolicy function accurately enforces the release sequencing policy
The logic effectively checks if a release is newer than the last active release for a given deployment and environment. The use of lodash chaining enhances readability, and the function handles edge cases appropriately.
Summary by CodeRabbit