feat: deployment dependency can also filter by versions#964
Merged
adityachoudhari26 merged 4 commits intomainfrom Apr 10, 2026
Merged
feat: deployment dependency can also filter by versions#964adityachoudhari26 merged 4 commits intomainfrom
adityachoudhari26 merged 4 commits intomainfrom
Conversation
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 0 minutes and 37 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefactors deployment dependency evaluation from job-based checks to evaluating currently deployed versions via CEL. Adds a top-level Changes
Sequence DiagramsequenceDiagram
participant Eval as Deployment Dependency Evaluator
participant RTG as ReleaseTarget Getter
participant DB as Postgres DB
participant CEL as CEL Engine
Note over Eval,CEL: Version-based dependency evaluation
Eval->>Eval: Compile `dependsOn` as CEL program
Eval->>RTG: GetReleaseTargetsForResource(resource)
RTG-->>Eval: [releaseTarget1, releaseTarget2, ...]
loop for each release target
Eval->>RTG: GetCurrentlyDeployedVersion(ctx, releaseTarget)
RTG->>DB: Query current release/version by ids
DB-->>RTG: row with version fields
RTG-->>Eval: DeploymentVersion {id, tag, name, status, metadata, createdAt}
Eval->>Eval: Build context { deployment: {...}, version: {...} }
Eval->>CEL: Evaluate `dependsOn` with context
CEL-->>Eval: true / false / error
alt true
Eval-->>Eval: Return AllowedResult (first match)
else false or error
Note over Eval: Continue to next release target (collect errors)
end
end
alt at least one matched
Eval-->>Eval: Allow deployment
else none matched
Eval-->>Eval: Deny deployment (include aggregated CEL errors if any)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Pull request overview
Adds version-aware evaluation to deployment dependency policies by exposing the currently deployed upstream version to the CEL selector, and updates generated OpenAPI/SDK artifacts, docs, and tests accordingly.
Changes:
- Extend CEL selector context with
version.*and document available variables fordependsOn. - Update deployment dependency evaluator to resolve each same-resource release target’s currently deployed version (latest successful job) and evaluate
dependsOnagainst{deployment, version}. - Update workspace-engine tests, mocks, and generated OpenAPI/SDK types to match the new behavior.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/workspace-engine-sdk/src/schema.ts | Updates SDK schema docstring for dependsOn to include version.* fields. |
| docs/policies/deployment-dependency.mdx | Documents new version.* variables and adds version-scoped dependency examples. |
| apps/workspace-engine/test/controllers/policy_deployment_dependency_test.go | Updates controller tests to use “currently deployed version” instead of “latest completed job” and adds version selector cases. |
| apps/workspace-engine/test/controllers/policy_combined_test.go | Updates combined-policy test harness data to use deployed versions. |
| apps/workspace-engine/test/controllers/harness/mocks.go | Renames mock storage from latest jobs to deployed versions and updates getter signature. |
| apps/workspace-engine/svc/controllers/desiredrelease/reconcile_test.go | Updates reconcile test mock getter signature. |
| apps/workspace-engine/svc/controllers/desiredrelease/policyeval/policyeval_test.go | Updates policy eval test mock getter signature. |
| apps/workspace-engine/svc/controllers/desiredrelease/policyeval/getter_postgres.go | Wires postgres policyeval getter to the new deployed-version getter. |
| apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/deploymentdependency/getter.go | Implements GetCurrentlyDeployedVersion using GetCurrentReleaseByReleaseTarget. |
| apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/deploymentdependency/deployment_dependency.go | Evaluates dependency selector against {deployment, version} for each same-resource release target. |
| apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/deploymentdependency/deployment_dependency_test.go | Updates evaluator unit tests for version-aware selectors and “any match” semantics. |
| apps/workspace-engine/pkg/selector/langs/cel/cel.go | Adds version as a CEL variable and mapping for DeploymentVersion. |
| apps/workspace-engine/pkg/selector/langs/cel/cel_test.go | Updates selector context tests to include version. |
| apps/workspace-engine/pkg/oapi/oapi.gen.go | Updates generated model comment for DeploymentDependencyRule.dependsOn. |
| apps/workspace-engine/oapi/spec/schemas/policy.jsonnet | Updates OpenAPI schema description for dependsOn. |
| apps/workspace-engine/oapi/openapi.json | Regenerates OpenAPI JSON with updated dependsOn description. |
| apps/api/src/types/openapi.ts | Updates API OpenAPI-generated TS types with updated dependsOn description. |
| apps/api/openapi/schemas/policies.jsonnet | Updates API OpenAPI schema description for dependsOn. |
| apps/api/openapi/openapi.json | Regenerates API OpenAPI JSON with updated dependsOn description. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+68
to
84
| row, err := p.queries.GetCurrentReleaseByReleaseTarget( | ||
| ctx, | ||
| db.GetCurrentReleaseByReleaseTargetParams{ | ||
| ResourceID: uuid.MustParse(rt.ResourceId), | ||
| EnvironmentID: uuid.MustParse(rt.EnvironmentId), | ||
| DeploymentID: uuid.MustParse(rt.DeploymentId), | ||
| }, | ||
| ) | ||
| if err != nil { | ||
| slog.Error( | ||
| "failed to get latest completed job for release target", | ||
| "failed to get current release for release target", | ||
| "releaseTarget", | ||
| releaseTarget.Key(), | ||
| rt.Key(), | ||
| "error", | ||
| err, | ||
| ) | ||
| return nil |
There was a problem hiding this comment.
GetCurrentReleaseByReleaseTarget will return pgx.ErrNoRows when a release target has no successful completed job yet, which is an expected/normal state for dependency evaluation. Logging this at error level will likely be noisy and can mask real errors. Consider treating ErrNoRows as a nil deployed version (no log, or debug-level) and only logging unexpected errors.
Comment on lines
+78
to
+94
| for _, rt := range releaseTargets { | ||
| deployment, err := e.getters.GetDeployment(ctx, rt.DeploymentId) | ||
| if err != nil || deployment == nil { | ||
| continue | ||
| } | ||
|
|
||
| version := e.getters.GetCurrentlyDeployedVersion(ctx, rt) | ||
| if version == nil { | ||
| continue | ||
| } | ||
|
|
||
| for _, upstreamReleaseTarget := range upstreamReleaseTargets { | ||
| if !e.checkUpstreamTargetHasSuccessfulRelease(upstreamReleaseTarget) { | ||
| return results.NewDeniedResult( | ||
| celCtx := cel.BuildEntityContext(nil, deployment, nil) | ||
| celCtx["version"] = cel.DeploymentVersionToMap(version) | ||
| matched, err := celutil.EvalBool(program, celCtx) | ||
| if err != nil { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Errors from GetDeployment / GetCurrentlyDeployedVersion / CEL evaluation are silently ignored via continue. When the selector is invalid for the provided context (e.g., missing fields) or the DB call fails, the final denied message becomes misleading (“no upstream…matches”) and makes debugging difficult. Consider recording these errors (span/log) and/or returning a denied result that includes the first evaluation error and the release target key.
Comment on lines
+76
to
+83
| releaseTargets := e.getters.GetReleaseTargetsForResource(ctx, scope.Resource.Id) | ||
|
|
||
| for _, rt := range releaseTargets { | ||
| deployment, err := e.getters.GetDeployment(ctx, rt.DeploymentId) | ||
| if err != nil || deployment == nil { | ||
| continue | ||
| } | ||
|
|
There was a problem hiding this comment.
This implementation does two DB lookups per release target (GetDeployment + GetCurrentlyDeployedVersion). If a resource has many release targets, this can become an N+1 query pattern during policy evaluation. Consider fetching deployments in bulk (e.g., GetAllDeployments once per workspace) and/or adding a batch query that returns {deployment, currentVersion} for all release targets on a resource.
Suggested change
| releaseTargets := e.getters.GetReleaseTargetsForResource(ctx, scope.Resource.Id) | |
| for _, rt := range releaseTargets { | |
| deployment, err := e.getters.GetDeployment(ctx, rt.DeploymentId) | |
| if err != nil || deployment == nil { | |
| continue | |
| } | |
| releaseTargets := e.getters.GetReleaseTargetsForResource(ctx, scope.Resource.Id) | |
| deploymentsByID := make(map[string]*oapi.Deployment, len(releaseTargets)) | |
| for _, rt := range releaseTargets { | |
| if _, ok := deploymentsByID[rt.DeploymentId]; ok { | |
| continue | |
| } | |
| deployment, err := e.getters.GetDeployment(ctx, rt.DeploymentId) | |
| if err != nil || deployment == nil { | |
| continue | |
| } | |
| deploymentsByID[rt.DeploymentId] = deployment | |
| } | |
| for _, rt := range releaseTargets { | |
| deployment, ok := deploymentsByID[rt.DeploymentId] | |
| if !ok || deployment == nil { | |
| continue | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/workspace-engine/pkg/selector/langs/cel/cel.go`:
- Line 14: CelSelector.Matches currently omits the "version" variable from the
activation even though the CEL environment and structToMap now support
oapi.DeploymentVersion; update CelSelector.Matches to include "version" in the
activation construction and populate it when a deployment version entity is
provided (use structToMap(oapi.DeploymentVersion) or equivalent conversion), so
that WithMapVariables("resource","deployment","environment","version") can
evaluate selectors using the version field consistently with Compile and
structToMap.
In
`@apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/deploymentdependency/deployment_dependency.go`:
- Around line 76-84: The loop over releaseTargets returned by
e.getters.GetReleaseTargetsForResource(ctx, scope.Resource.Id) must skip the
release target that's currently being evaluated to avoid self-satisfaction;
inside the for _, rt := range releaseTargets loop (before calling
e.getters.GetDeployment or e.getters.GetCurrentlyDeployedVersion) add a guard
that continues when rt identifies the current target (e.g., compare rt.Id to
scope.Target.Id or otherwise the unique identifier you use for the evaluated
release target) so the current release target is not considered as an upstream
dependency.
In
`@apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/deploymentdependency/getter.go`:
- Around line 76-84: GetCurrentReleaseByReleaseTarget currently logs
pgx.ErrNoRows as an error for empty deployments; update the error handling in
the function (the block that checks err after calling the DB) to first detect
the sentinel with if errors.Is(err, pgx.ErrNoRows) { return nil } and only call
slog.Error("failed to get current release for release target", "releaseTarget",
rt.Key(), "error", err) for other errors so expected empty state does not
produce error logs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 279422d7-21ca-4156-9ecb-e3df0e6a30a5
📒 Files selected for processing (19)
apps/api/openapi/openapi.jsonapps/api/openapi/schemas/policies.jsonnetapps/api/src/types/openapi.tsapps/workspace-engine/oapi/openapi.jsonapps/workspace-engine/oapi/spec/schemas/policy.jsonnetapps/workspace-engine/pkg/oapi/oapi.gen.goapps/workspace-engine/pkg/selector/langs/cel/cel.goapps/workspace-engine/pkg/selector/langs/cel/cel_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/deploymentdependency/deployment_dependency.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/deploymentdependency/deployment_dependency_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/deploymentdependency/getter.goapps/workspace-engine/svc/controllers/desiredrelease/policyeval/getter_postgres.goapps/workspace-engine/svc/controllers/desiredrelease/policyeval/policyeval_test.goapps/workspace-engine/svc/controllers/desiredrelease/reconcile_test.goapps/workspace-engine/test/controllers/harness/mocks.goapps/workspace-engine/test/controllers/policy_combined_test.goapps/workspace-engine/test/controllers/policy_deployment_dependency_test.godocs/policies/deployment-dependency.mdxpackages/workspace-engine-sdk/src/schema.ts
.../pkg/workspace/releasemanager/policy/evaluator/deploymentdependency/deployment_dependency.go
Show resolved
Hide resolved
...orkspace-engine/pkg/workspace/releasemanager/policy/evaluator/deploymentdependency/getter.go
Show resolved
Hide resolved
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #743
Summary by CodeRabbit
New Features
Documentation