feat: release target eligible versions v1 endpoint#1143
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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 (2)
📝 WalkthroughWalkthroughThis PR introduces a new API endpoint ChangesEligible Versions Listing Endpoint
Sequence Diagram(s)sequenceDiagram
participant Client
participant APIGateway as API Gateway
participant WorkspaceEngine as Handler
participant CELEnv as CEL Compiler
participant PolicyEval as Policy Evaluation
participant Store as Release Target Store
Client->>APIGateway: POST /v1/workspaces/{id}/release-targets/{key}/eligible-versions<br/>filter: "version.tag == 'v1.0'", limit, offset
APIGateway->>WorkspaceEngine: forward request
WorkspaceEngine->>CELEnv: compile filter expression
CELEnv-->>WorkspaceEngine: compiled filter AST
WorkspaceEngine->>Store: fetch release target
Store-->>WorkspaceEngine: release target data
WorkspaceEngine->>PolicyEval: ListDeployableVersions(versions, evaluators)
PolicyEval-->>WorkspaceEngine: eligible versions (policy-filtered)
WorkspaceEngine->>WorkspaceEngine: evaluate CEL on each version
WorkspaceEngine->>WorkspaceEngine: apply pagination (limit, offset)
WorkspaceEngine-->>APIGateway: 200 { items, total, limit, offset }
APIGateway-->>Client: paginated eligible versions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new v1 endpoint POST /v1/workspaces/{workspaceId}/release-targets/{releaseTargetKey}/eligible-versions that returns deployment versions which pass every policy rule for a given release target, with optional CEL post-filtering and pagination. Generated OpenAPI / SDK / API-gateway code and types are regenerated to expose the route, and a new policyeval.ListDeployableVersions helper provides the non-short-circuiting evaluator iteration.
Changes:
- New workspace-engine handler
ListEligibleVersionsForReleaseTargetpluspolicyeval.ListDeployableVersionshelper (with unit tests). - OpenAPI spec + regenerated Go/TS bindings, and
apps/apiproxy route forwarding to the workspace-engine. - New Playwright e2e suite covering happy path, policy-blocked versions, CEL filtering, pagination, 404, and malformed-CEL 400.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/workspace-engine/svc/http/server/openapi/release_targets/eligible_versions.go | New Gin handler: parses key, validates/compiles CEL, runs policy evaluator, post-filters and paginates. |
| apps/workspace-engine/svc/controllers/desiredrelease/policyeval/policyeval.go | Adds ListDeployableVersions that returns all allowed versions instead of first match. |
| apps/workspace-engine/svc/controllers/desiredrelease/policyeval/policyeval_test.go | Unit tests for the new helper covering empty, allow, deny, skip, and error cases. |
| apps/workspace-engine/pkg/oapi/oapi.gen.go | Regenerated server interface, params types, and router registration. |
| apps/workspace-engine/oapi/openapi.json | Regenerated spec for the new endpoint. |
| apps/workspace-engine/oapi/spec/paths/release_targets.jsonnet | Jsonnet source for the new endpoint spec. |
| apps/api/src/routes/v1/workspaces/release-targets.ts | API-gateway handler proxying the new endpoint to workspace-engine. |
| apps/api/openapi/openapi.json | Regenerated apps/api spec. |
| apps/api/openapi/paths/release-targets.jsonnet | Jsonnet source for the apps/api endpoint spec. |
| apps/api/src/types/openapi.ts | Regenerated TS types for apps/api. |
| packages/workspace-engine-sdk/src/schema.ts | Regenerated SDK types. |
| e2e/api/schema.ts | Regenerated e2e schema bindings. |
| e2e/tests/api/release-target-eligible-versions.spec.ts | New Playwright suite for the endpoint. |
| e2e/package.json | New script to run the new e2e suite. |
Files not reviewed (1)
- apps/workspace-engine/pkg/oapi/oapi.gen.go: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+130
to
+141
| limit := 50 | ||
| if params.Limit != nil { | ||
| limit = *params.Limit | ||
| } | ||
| offset := 0 | ||
| if params.Offset != nil { | ||
| offset = *params.Offset | ||
| } | ||
|
|
||
| total := len(filtered) | ||
| start := min(offset, total) | ||
| end := min(start+limit, total) |
Comment on lines
+57
to
+65
| if err := celEnv.Validate(filter); err != nil { | ||
| c.JSON(http.StatusBadRequest, gin.H{"error": "invalid filter expression: " + err.Error()}) | ||
| return | ||
| } | ||
| prg, err := celEnv.Compile(filter) | ||
| if err != nil { | ||
| c.JSON(http.StatusBadRequest, gin.H{"error": "invalid filter expression: " + err.Error()}) | ||
| return | ||
| } |
Comment on lines
+104
to
+128
| evals := policyeval.CollectEvaluators(ctx, getter, oapiRT, rtPolicies) | ||
| versions := getter.IterCandidateVersions(ctx, drt.DeploymentID, nil, nil) | ||
|
|
||
| eligible, err := policyeval.ListDeployableVersions(ctx, getter, oapiRT, versions, evals, *scope) | ||
| if err != nil { | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": "list deployable versions: " + err.Error()}) | ||
| return | ||
| } | ||
|
|
||
| filtered := make([]*oapi.DeploymentVersion, 0, len(eligible)) | ||
| for _, v := range eligible { | ||
| vMap, err := celutil.EntityToMap(v) | ||
| if err != nil { | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": "convert version: " + err.Error()}) | ||
| return | ||
| } | ||
| ok, err := celutil.EvalBool(prg, map[string]any{"version": vMap}) | ||
| if err != nil { | ||
| c.JSON(http.StatusBadRequest, gin.H{"error": "evaluate filter: " + err.Error()}) | ||
| return | ||
| } | ||
| if ok { | ||
| filtered = append(filtered, v) | ||
| } | ||
| } |
![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: 5
🧹 Nitpick comments (2)
apps/api/openapi/openapi.json (1)
7547-7550: ⚡ Quick winDeclare
filterdefault in schema, not only in description.The description says
filterdefaults to"true", but the schema omitsdefault, so generated clients/docs may not reflect that behavior.Suggested OpenAPI diff
"filter": { "description": "CEL expression to filter eligible versions. Defaults to \"true\" (all eligible versions).", + "default": "true", "type": "string" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/openapi/openapi.json` around lines 7547 - 7550, The OpenAPI schema for the property named "filter" declares in its description that it defaults to "true" but does not include a JSON Schema default; update the schema entry for the "filter" property to include "default": "true" (i.e., add the default field alongside "description" and "type") so generated docs/clients pick up the actual default value for the filter property.e2e/tests/api/release-target-eligible-versions.spec.ts (1)
214-235: 🏗️ Heavy liftAdd a dependency-aware CEL filter test case.
Given issue
#1125’s objective, this suite should validate CEL access to version dependency data (not onlyversion.tag) to prevent regressions on the primary feature.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/tests/api/release-target-eligible-versions.spec.ts` around lines 214 - 235, Add a new e2e test to validate CEL filters can access version dependency data: create versions via insertVersions that include a version with a dependency (e.g., a dependency object in the version payload), call the same endpoint via api.POST to "/v1/workspaces/{workspaceId}/release-targets/{releaseTargetKey}/eligible-versions" with a CEL filter that references dependency fields (for example using dependency existence or dependency.version/tag checks), and assert the response total and items only include versions matching the dependency-aware CEL expression; reference the existing test structure (insertVersions, api.POST, workspace.id, releaseTargetKey) to mirror setup and assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/routes/v1/workspaces/release-targets.ts`:
- Around line 245-255: The code destructures filter from req.body which can be
undefined and cause a TypeError; update the destructuring to guard against a
missing body (e.g., use a fallback like req.body || {} or optional chaining) so
that const { filter } = ... never throws, and keep the rest of the POST call to
getClientFor(...).POST unchanged.
In `@apps/api/src/types/openapi.ts`:
- Around line 5024-5031: The OpenAPI output marks requestBody as required even
though the CEL filter is optional; update the OpenAPI source spec to make the
request body optional (set requestBody.required: false or remove the required
flag) so the generated type for requestBody is optional, then run the project
OpenAPI generation command to regenerate src/types/openapi.ts; focus your change
around the requestBody and filter schema in the endpoint definition before
regenerating.
In
`@apps/workspace-engine/svc/http/server/openapi/release_targets/eligible_versions.go`:
- Around line 130-142: Validate and clamp pagination inputs before slicing:
ensure params.Limit and params.Offset are checked for non-nil and non-negative,
set offset = max(0, *params.Offset) and limit = max(0, *params.Limit) (with a
sensible default), then compute start := min(offset, total) and end :=
min(start+limit, total) while guarding against integer overflow when computing
start+limit (e.g., use if limit > total-start { end = total } else { end =
start+limit }). Update the code around variables limit, offset, start, end and
the slice filtered[start:end] to use these validated/clamped values.
In `@e2e/tests/api/release-target-eligible-versions.spec.ts`:
- Around line 35-60: The test makes several API calls using api.PUT and
api.DELETE without asserting their response statuses, which can silently corrupt
fixtures; update the calls around
api.PUT("/v1/workspaces/{workspaceId}/systems/{systemId}/deployments/{deploymentId}")
and the subsequent api.PUT that attaches the environment (and the similar calls
at lines ~107-109) to capture the response objects and assert response.status
(e.g., expect(res.response.status).toBe(200) or the appropriate code) before
proceeding, and do the same for the final api.DELETE teardown so failures fail
fast and prevent misleading test errors; reference the api.PUT and api.DELETE
invocations and the environmentId assignment to locate where to add these
assertions.
- Around line 17-117: The test setup in
test.beforeAll/test.afterAll/test.afterEach imperatively creates resources via
API and DB inserts (systemId, deploymentId, environmentId, resourceId,
releaseTargetKey) which violates the repo E2E fixture pattern; replace this with
a YAML fixture (.spec.yaml) and load it using importEntitiesFromYaml in your
test.beforeAll to create the system/deployment/environment/resource with
randomized prefixes, then remove the manual DB inserts and API teardown and call
cleanupImportedEntities in test.afterAll/afterEach to delete created entities;
ensure the fixture declares computedDeploymentResource and
computedEnvironmentResource links (or use a fixture helper to insert those) so
releaseTargetKey logic continues to work.
---
Nitpick comments:
In `@apps/api/openapi/openapi.json`:
- Around line 7547-7550: The OpenAPI schema for the property named "filter"
declares in its description that it defaults to "true" but does not include a
JSON Schema default; update the schema entry for the "filter" property to
include "default": "true" (i.e., add the default field alongside "description"
and "type") so generated docs/clients pick up the actual default value for the
filter property.
In `@e2e/tests/api/release-target-eligible-versions.spec.ts`:
- Around line 214-235: Add a new e2e test to validate CEL filters can access
version dependency data: create versions via insertVersions that include a
version with a dependency (e.g., a dependency object in the version payload),
call the same endpoint via api.POST to
"/v1/workspaces/{workspaceId}/release-targets/{releaseTargetKey}/eligible-versions"
with a CEL filter that references dependency fields (for example using
dependency existence or dependency.version/tag checks), and assert the response
total and items only include versions matching the dependency-aware CEL
expression; reference the existing test structure (insertVersions, api.POST,
workspace.id, releaseTargetKey) to mirror setup and assertions.
🪄 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: 76ccb990-6f14-4d47-bbd4-9211faab1a3d
📒 Files selected for processing (14)
apps/api/openapi/openapi.jsonapps/api/openapi/paths/release-targets.jsonnetapps/api/src/routes/v1/workspaces/release-targets.tsapps/api/src/types/openapi.tsapps/workspace-engine/oapi/openapi.jsonapps/workspace-engine/oapi/spec/paths/release_targets.jsonnetapps/workspace-engine/pkg/oapi/oapi.gen.goapps/workspace-engine/svc/controllers/desiredrelease/policyeval/policyeval.goapps/workspace-engine/svc/controllers/desiredrelease/policyeval/policyeval_test.goapps/workspace-engine/svc/http/server/openapi/release_targets/eligible_versions.goe2e/api/schema.tse2e/package.jsone2e/tests/api/release-target-eligible-versions.spec.tspackages/workspace-engine-sdk/src/schema.ts
Comment on lines
+245
to
+255
| const { filter } = req.body; | ||
|
|
||
| const { data, error, response } = await getClientFor(workspaceId).POST( | ||
| "/v1/workspaces/{workspaceId}/release-targets/{releaseTargetKey}/eligible-versions", | ||
| { | ||
| params: { | ||
| path: { workspaceId, releaseTargetKey }, | ||
| query: { limit, offset }, | ||
| }, | ||
| body: { filter }, | ||
| }, |
Contributor
There was a problem hiding this comment.
Guard optional request body before destructuring.
Line 245 can throw a runtime TypeError when clients omit the request body. This endpoint supports optional filtering, so body access should be null-safe.
Suggested fix
- const { filter } = req.body;
+ const filter = req.body?.filter;
...
- body: { filter },
+ ...(filter !== undefined ? { body: { filter } } : {}),📝 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.
Suggested change
| const { filter } = req.body; | |
| const { data, error, response } = await getClientFor(workspaceId).POST( | |
| "/v1/workspaces/{workspaceId}/release-targets/{releaseTargetKey}/eligible-versions", | |
| { | |
| params: { | |
| path: { workspaceId, releaseTargetKey }, | |
| query: { limit, offset }, | |
| }, | |
| body: { filter }, | |
| }, | |
| const filter = req.body?.filter; | |
| const { data, error, response } = await getClientFor(workspaceId).POST( | |
| "/v1/workspaces/{workspaceId}/release-targets/{releaseTargetKey}/eligible-versions", | |
| { | |
| params: { | |
| path: { workspaceId, releaseTargetKey }, | |
| query: { limit, offset }, | |
| }, | |
| ...(filter !== undefined ? { body: { filter } } : {}), | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/routes/v1/workspaces/release-targets.ts` around lines 245 - 255,
The code destructures filter from req.body which can be undefined and cause a
TypeError; update the destructuring to guard against a missing body (e.g., use a
fallback like req.body || {} or optional chaining) so that const { filter } =
... never throws, and keep the rest of the POST call to getClientFor(...).POST
unchanged.
Comment on lines
+5024
to
+5031
| requestBody: { | ||
| content: { | ||
| "application/json": { | ||
| /** @description CEL expression to filter eligible versions. Defaults to "true" (all eligible versions). */ | ||
| filter?: string; | ||
| }; | ||
| }; | ||
| }; |
Contributor
There was a problem hiding this comment.
Request body is modeled as required despite optional filter semantics.
requestBody is required here, which forces clients to send {} even when no CEL filter is provided. That conflicts with the endpoint description and PR intent of optional body-based filtering. Make the request body optional in the OpenAPI source spec (required: false or omitted) and regenerate this file.
As per coding guidelines, "Do not hand-edit generated OpenAPI output (src/types/openapi.ts and openapi/openapi.json). Regenerate using the generate command instead."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/types/openapi.ts` around lines 5024 - 5031, The OpenAPI output
marks requestBody as required even though the CEL filter is optional; update the
OpenAPI source spec to make the request body optional (set requestBody.required:
false or remove the required flag) so the generated type for requestBody is
optional, then run the project OpenAPI generation command to regenerate
src/types/openapi.ts; focus your change around the requestBody and filter schema
in the endpoint definition before regenerating.
Comment on lines
+17
to
+117
| test.beforeAll(async ({ api, workspace }) => { | ||
| const systemRes = await api.POST("/v1/workspaces/{workspaceId}/systems", { | ||
| params: { path: { workspaceId: workspace.id } }, | ||
| body: { name: `evt-system-${faker.string.alphanumeric(8)}` }, | ||
| }); | ||
| expect(systemRes.response.status).toBe(202); | ||
| systemId = systemRes.data!.id; | ||
|
|
||
| const depName = `evt-dep-${faker.string.alphanumeric(8)}`; | ||
| const depRes = await api.POST( | ||
| "/v1/workspaces/{workspaceId}/deployments", | ||
| { | ||
| params: { path: { workspaceId: workspace.id } }, | ||
| body: { name: depName, slug: depName }, | ||
| }, | ||
| ); | ||
| expect(depRes.response.status).toBe(202); | ||
| deploymentId = depRes.data!.id; | ||
| await api.PUT( | ||
| "/v1/workspaces/{workspaceId}/systems/{systemId}/deployments/{deploymentId}", | ||
| { | ||
| params: { | ||
| path: { workspaceId: workspace.id, systemId, deploymentId }, | ||
| }, | ||
| }, | ||
| ); | ||
|
|
||
| const envRes = await api.POST( | ||
| "/v1/workspaces/{workspaceId}/environments", | ||
| { | ||
| params: { path: { workspaceId: workspace.id } }, | ||
| body: { name: `evt-env-${faker.string.alphanumeric(8)}` }, | ||
| }, | ||
| ); | ||
| expect(envRes.response.status).toBe(202); | ||
| environmentId = envRes.data!.id; | ||
| await api.PUT( | ||
| "/v1/workspaces/{workspaceId}/systems/{systemId}/environments/{environmentId}", | ||
| { | ||
| params: { | ||
| path: { workspaceId: workspace.id, systemId, environmentId }, | ||
| }, | ||
| }, | ||
| ); | ||
|
|
||
| const identifier = `evt-res-${faker.string.alphanumeric(8)}`; | ||
| const putRes = await api.PUT( | ||
| "/v1/workspaces/{workspaceId}/resources/identifier/{identifier}", | ||
| { | ||
| params: { path: { workspaceId: workspace.id, identifier } }, | ||
| body: { | ||
| name: identifier, | ||
| kind: "TestKind", | ||
| version: "1.0.0", | ||
| config: {}, | ||
| metadata: {}, | ||
| }, | ||
| }, | ||
| ); | ||
| expect(putRes.response.status).toBe(202); | ||
|
|
||
| const getResourceRes = await api.GET( | ||
| "/v1/workspaces/{workspaceId}/resources/identifier/{identifier}", | ||
| { params: { path: { workspaceId: workspace.id, identifier } } }, | ||
| ); | ||
| expect(getResourceRes.response.status).toBe(200); | ||
| resourceId = getResourceRes.data!.id; | ||
|
|
||
| // Bypass reconciliation: insert computed_*_resource link rows directly so | ||
| // the (resource, environment, deployment) triple is a valid release target | ||
| // without waiting on the workspace-engine to fan things out. | ||
| const now = new Date(); | ||
| await db | ||
| .insert(schema.computedDeploymentResource) | ||
| .values({ deploymentId, resourceId, lastEvaluatedAt: now }); | ||
| await db | ||
| .insert(schema.computedEnvironmentResource) | ||
| .values({ environmentId, resourceId, lastEvaluatedAt: now }); | ||
|
|
||
| releaseTargetKey = `${resourceId}-${environmentId}-${deploymentId}`; | ||
| }); | ||
|
|
||
| test.afterAll(async ({ api, workspace }) => { | ||
| // FK cascade from deployment → deployment_version was dropped in migration | ||
| // 0139 (see deployment-version-list.spec.ts), so versions need explicit | ||
| // delete before the deployment goes. | ||
| await db | ||
| .delete(schema.deploymentVersion) | ||
| .where(eq(schema.deploymentVersion.deploymentId, deploymentId)); | ||
|
|
||
| await api.DELETE("/v1/workspaces/{workspaceId}/systems/{systemId}", { | ||
| params: { path: { workspaceId: workspace.id, systemId } }, | ||
| }); | ||
| }); | ||
|
|
||
| test.afterEach(async () => { | ||
| await db | ||
| .delete(schema.deploymentVersion) | ||
| .where(eq(schema.deploymentVersion.deploymentId, deploymentId)); | ||
| }); | ||
|
|
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Migrate this E2E setup to YAML fixtures + fixture helpers.
This spec provisions and tears down entities imperatively (API + direct DB), which diverges from the repository’s required E2E fixture pattern and increases state-leak risk. Please move entities into a .spec.yaml and use importEntitiesFromYaml/cleanupImportedEntities (with random prefixing when runs may overlap).
As per coding guidelines, **/e2e/**/*.spec.ts: E2E tests use YAML fixture files (.spec.yaml alongside .spec.ts) to declare test entities; use importEntitiesFromYaml to load them and cleanupImportedEntities to tear them down.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/tests/api/release-target-eligible-versions.spec.ts` around lines 17 -
117, The test setup in test.beforeAll/test.afterAll/test.afterEach imperatively
creates resources via API and DB inserts (systemId, deploymentId, environmentId,
resourceId, releaseTargetKey) which violates the repo E2E fixture pattern;
replace this with a YAML fixture (.spec.yaml) and load it using
importEntitiesFromYaml in your test.beforeAll to create the
system/deployment/environment/resource with randomized prefixes, then remove the
manual DB inserts and API teardown and call cleanupImportedEntities in
test.afterAll/afterEach to delete created entities; ensure the fixture declares
computedDeploymentResource and computedEnvironmentResource links (or use a
fixture helper to insert those) so releaseTargetKey logic continues to work.
Comment on lines
+35
to
+60
| await api.PUT( | ||
| "/v1/workspaces/{workspaceId}/systems/{systemId}/deployments/{deploymentId}", | ||
| { | ||
| params: { | ||
| path: { workspaceId: workspace.id, systemId, deploymentId }, | ||
| }, | ||
| }, | ||
| ); | ||
|
|
||
| const envRes = await api.POST( | ||
| "/v1/workspaces/{workspaceId}/environments", | ||
| { | ||
| params: { path: { workspaceId: workspace.id } }, | ||
| body: { name: `evt-env-${faker.string.alphanumeric(8)}` }, | ||
| }, | ||
| ); | ||
| expect(envRes.response.status).toBe(202); | ||
| environmentId = envRes.data!.id; | ||
| await api.PUT( | ||
| "/v1/workspaces/{workspaceId}/systems/{systemId}/environments/{environmentId}", | ||
| { | ||
| params: { | ||
| path: { workspaceId: workspace.id, systemId, environmentId }, | ||
| }, | ||
| }, | ||
| ); |
Contributor
There was a problem hiding this comment.
Assert setup/teardown API responses to avoid silent fixture corruption.
The api.PUT link calls and final api.DELETE are not status-checked. If one fails, later assertions can fail for the wrong reason.
Suggested fix
await api.PUT(
"/v1/workspaces/{workspaceId}/systems/{systemId}/deployments/{deploymentId}",
{
params: {
path: { workspaceId: workspace.id, systemId, deploymentId },
},
},
);
+ // assert link succeeded
+ // expect(linkDepRes.response.status).toBe(200);
...
await api.PUT(
"/v1/workspaces/{workspaceId}/systems/{systemId}/environments/{environmentId}",
{
params: {
path: { workspaceId: workspace.id, systemId, environmentId },
},
},
);
+ // expect(linkEnvRes.response.status).toBe(200);
...
- await api.DELETE("/v1/workspaces/{workspaceId}/systems/{systemId}", {
+ const deleteSystemRes = await api.DELETE("/v1/workspaces/{workspaceId}/systems/{systemId}", {
params: { path: { workspaceId: workspace.id, systemId } },
});
+ expect(deleteSystemRes.response.status).toBe(204);Also applies to: 107-109
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/tests/api/release-target-eligible-versions.spec.ts` around lines 35 - 60,
The test makes several API calls using api.PUT and api.DELETE without asserting
their response statuses, which can silently corrupt fixtures; update the calls
around
api.PUT("/v1/workspaces/{workspaceId}/systems/{systemId}/deployments/{deploymentId}")
and the subsequent api.PUT that attaches the environment (and the similar calls
at lines ~107-109) to capture the response objects and assert response.status
(e.g., expect(res.response.status).toBe(200) or the appropriate code) before
proceeding, and do the same for the final api.DELETE teardown so failures fail
fast and prevent misleading test errors; reference the api.PUT and api.DELETE
invocations and the environmentId assignment to locate where to add these
assertions.
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.
fixes #1125
Summary by CodeRabbit
New Features
Tests