fix: dry run dispatch ctx includes synthetic release#1000
fix: dry run dispatch ctx includes synthetic release#1000adityachoudhari26 merged 2 commits intomainfrom
Conversation
|
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 54 minutes and 8 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)
📝 WalkthroughWalkthroughA new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 a synthetic Release object to the deployment-plan dispatch context so dry-run/plan job agents can template against dispatchCtx.release similarly to real job dispatch.
Changes:
- Construct a synthetic
oapi.Releaseduring deployment plan target processing. - Attach the synthetic release to
oapi.DispatchContextbefore persisting the dispatch context blob.
Comments suppressed due to low confidence (1)
apps/workspace-engine/svc/controllers/deploymentplan/controller.go:214
- There is existing test coverage asserting the marshalled/unmarshalled dispatch context fields for this controller, but it doesn't validate the newly-added
dispatchCtx.Releasebehavior. Add assertions to the dispatch-context test to ensureReleaseis non-nil and that it contains the expected target/version/variables (and an emptyencryptedVariablesarray) so regressions are caught.
dispatchCtx := &oapi.DispatchContext{
Deployment: deployment,
Environment: env,
Resource: resource,
Version: version,
Release: release,
Variables: &variables,
JobAgent: *agent,
JobAgentConfig: mergedConfig,
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Variables: variables, | ||
| Version: *version, |
There was a problem hiding this comment.
oapi.Release has a required encryptedVariables field (no omitempty). Since the synthetic release leaves it unset, JSON marshalling will emit "encryptedVariables": null, which can break template usage (e.g., range over .release.encryptedVariables). Initialize it to an empty slice (and keep it non-nil) when building the synthetic release.
| Variables: variables, | |
| Version: *version, | |
| Variables: variables, | |
| EncryptedVariables: []oapi.EncryptedVariable{}, | |
| Version: *version, |
| release := &oapi.Release{ | ||
| CreatedAt: time.Now().Format(time.RFC3339), | ||
| Id: uuid.New(), | ||
| ReleaseTarget: oapi.ReleaseTarget{ | ||
| DeploymentId: plan.DeploymentID.String(), | ||
| EnvironmentId: target.EnvironmentID.String(), | ||
| ResourceId: target.ResourceID.String(), | ||
| }, | ||
| Variables: variables, | ||
| Version: *version, | ||
| } |
There was a problem hiding this comment.
The synthetic release is created inside the per-agent loop using time.Now() and a new UUID each iteration. This makes the persisted dispatch context non-deterministic across agents (and across retries) for the same plan target, which can lead to inconsistent plan outputs if templates reference release.id/release.createdAt. Consider creating the synthetic release once per target (outside the loop) and/or deriving a stable ID/timestamp from the plan/target so all agents see the same release metadata.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/svc/controllers/deploymentplan/controller.go`:
- Around line 193-203: The synthetic oapi.Release literal currently omits the
EncryptedVariables field; update the Release construction (the release variable
in controller.go) to set EncryptedVariables explicitly — use an empty slice
(e.g., empty []oapi.EncryptedVariable) when there are none or populate it by
mapping existing variables to the encrypted form if an encryption/derivation
helper exists (use the same keys/IDs as Variables). Ensure you reference and set
the EncryptedVariables field on the oapi.Release struct alongside Variables and
Version.
🪄 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: db4ac3c1-109e-4025-9e65-4c05a59bbea2
📒 Files selected for processing (1)
apps/workspace-engine/svc/controllers/deploymentplan/controller.go
| release := &oapi.Release{ | ||
| CreatedAt: time.Now().Format(time.RFC3339), | ||
| Id: uuid.New(), | ||
| ReleaseTarget: oapi.ReleaseTarget{ | ||
| DeploymentId: plan.DeploymentID.String(), | ||
| EnvironmentId: target.EnvironmentID.String(), | ||
| ResourceId: target.ResourceID.String(), | ||
| }, | ||
| Variables: variables, | ||
| Version: *version, | ||
| } |
There was a problem hiding this comment.
Populate Release.EncryptedVariables in the synthetic release payload.
The new oapi.Release literal omits EncryptedVariables, which makes this synthetic release incomplete against the generated release shape. Please set it explicitly (empty slice if none, or derived encrypted keys when available).
Proposed fix
release := &oapi.Release{
CreatedAt: time.Now().Format(time.RFC3339),
+ EncryptedVariables: []string{},
Id: uuid.New(),
ReleaseTarget: oapi.ReleaseTarget{
DeploymentId: plan.DeploymentID.String(),
EnvironmentId: target.EnvironmentID.String(),
ResourceId: target.ResourceID.String(),
},
Variables: variables,
Version: *version,
}📝 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.
| release := &oapi.Release{ | |
| CreatedAt: time.Now().Format(time.RFC3339), | |
| Id: uuid.New(), | |
| ReleaseTarget: oapi.ReleaseTarget{ | |
| DeploymentId: plan.DeploymentID.String(), | |
| EnvironmentId: target.EnvironmentID.String(), | |
| ResourceId: target.ResourceID.String(), | |
| }, | |
| Variables: variables, | |
| Version: *version, | |
| } | |
| release := &oapi.Release{ | |
| CreatedAt: time.Now().Format(time.RFC3339), | |
| EncryptedVariables: []string{}, | |
| Id: uuid.New(), | |
| ReleaseTarget: oapi.ReleaseTarget{ | |
| DeploymentId: plan.DeploymentID.String(), | |
| EnvironmentId: target.EnvironmentID.String(), | |
| ResourceId: target.ResourceID.String(), | |
| }, | |
| Variables: variables, | |
| Version: *version, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/svc/controllers/deploymentplan/controller.go` around
lines 193 - 203, The synthetic oapi.Release literal currently omits the
EncryptedVariables field; update the Release construction (the release variable
in controller.go) to set EncryptedVariables explicitly — use an empty slice
(e.g., empty []oapi.EncryptedVariable) when there are none or populate it by
mapping existing variables to the encrypted form if an encryption/derivation
helper exists (use the same keys/IDs as Variables). Ensure you reference and set
the EncryptedVariables field on the oapi.Release struct alongside Variables and
Version.
Summary by CodeRabbit
Release Notes