E2E: Allow yaml-entity creation order to be explicit #576
E2E: Allow yaml-entity creation order to be explicit #576jsbroks merged 9 commits intoctrlplanedev:mainfrom
Conversation
WalkthroughThe codebase refactors YAML entity import logic from a single asynchronous function into a modular Changes
Sequence Diagram(s)sequenceDiagram
participant TestSuite
participant EntitiesBuilder
participant ApiClient
participant WorkspaceFixture
TestSuite->>EntitiesBuilder: new EntitiesBuilder(api, workspace, yamlPath)
loop For each creation step
TestSuite->>EntitiesBuilder: createSystem()/createResources()/createEnvironments()/...
EntitiesBuilder->>ApiClient: POST entity creation request
ApiClient-->>EntitiesBuilder: Entity created response
EntitiesBuilder-->>EntitiesBuilder: Store created entity in cache
end
TestSuite->>EntitiesBuilder: Access builder.cache for entities
TestSuite->>EntitiesBuilder: cleanupImportedEntities(api, builder.cache, workspaceId)
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used📓 Path-based instructions (1)`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (9)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
e2e/api/yaml-loader.ts (1)
244-246: Address the TODO comment for resource selector.There's a TODO comment about updating the resource selector that should be addressed. If this functionality is needed, it should be implemented. Otherwise, consider removing the comment.
Would you like me to open an issue to track the implementation of resource selector updates for environments?
e2e/tests/api/deployment-variable.spec.ts (1)
207-207: Minor typo in test name.There's a typo: "shoudl" should be "should". This appears to be pre-existing code not introduced by this PR.
- test("shoudl fail if more than one default value is provided", async ({ api }) => { + test("should fail if more than one default value is provided", async ({ api }) => {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
e2e/api/yaml-loader.ts(11 hunks)e2e/tests/api/deployment-remove-event.spec.ts(5 hunks)e2e/tests/api/deployment-variable.spec.ts(4 hunks)e2e/tests/api/deployment-version.spec.ts(10 hunks)e2e/tests/api/deployments.spec.ts(15 hunks)e2e/tests/api/environments.spec.ts(16 hunks)e2e/tests/api/job-agents.spec.ts(2 hunks)e2e/tests/api/release-targets.spec.ts(2 hunks)e2e/tests/api/release.spec.ts(12 hunks)e2e/tests/api/resource-grouping.spec.ts(1 hunks)e2e/tests/api/resource-grouping.spec.yaml(0 hunks)e2e/tests/api/resource-relationships.spec.ts(13 hunks)e2e/tests/api/resource-variables.spec.ts(10 hunks)e2e/tests/api/resources.spec.ts(10 hunks)e2e/tests/api/yaml-import.spec.ts(3 hunks)
💤 Files with no reviewable changes (1)
- e2e/tests/api/resource-grouping.spec.yaml
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
e2e/tests/api/resources.spec.tse2e/tests/api/job-agents.spec.tse2e/tests/api/resource-grouping.spec.tse2e/tests/api/release-targets.spec.tse2e/tests/api/deployment-version.spec.tse2e/tests/api/yaml-import.spec.tse2e/tests/api/environments.spec.tse2e/tests/api/deployments.spec.tse2e/tests/api/deployment-variable.spec.tse2e/tests/api/resource-relationships.spec.tse2e/tests/api/deployment-remove-event.spec.tse2e/tests/api/release.spec.tse2e/tests/api/resource-variables.spec.tse2e/api/yaml-loader.ts
🧬 Code Graph Analysis (6)
e2e/tests/api/resource-grouping.spec.ts (2)
e2e/tests/fixtures.ts (1)
test(11-26)e2e/api/yaml-loader.ts (1)
EntitiesBuilder(128-435)
e2e/tests/api/environments.spec.ts (5)
e2e/tests/fixtures.ts (1)
test(11-26)e2e/api/yaml-loader.ts (2)
EntitiesBuilder(128-435)cleanupImportedEntities(442-490)packages/db/src/schema/workspace.ts (1)
workspace(18-27)packages/db/src/schema/environment.ts (1)
environment(59-84)packages/db/src/schema/release.ts (1)
releaseTarget(20-42)
e2e/tests/api/resource-relationships.spec.ts (3)
e2e/tests/fixtures.ts (1)
test(11-26)e2e/api/yaml-loader.ts (2)
EntitiesBuilder(128-435)cleanupImportedEntities(442-490)packages/db/src/schema/workspace.ts (1)
workspace(18-27)
e2e/tests/api/deployment-remove-event.spec.ts (1)
e2e/api/yaml-loader.ts (2)
EntitiesBuilder(128-435)cleanupImportedEntities(442-490)
e2e/tests/api/resource-variables.spec.ts (3)
e2e/tests/fixtures.ts (1)
test(11-26)e2e/api/yaml-loader.ts (2)
EntitiesBuilder(128-435)cleanupImportedEntities(442-490)packages/db/src/schema/workspace.ts (1)
workspace(18-27)
e2e/api/yaml-loader.ts (2)
e2e/api/index.ts (1)
ApiClient(23-23)e2e/tests/auth.setup.ts (1)
WorkspaceFixture(15-20)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Typecheck
- GitHub Check: Lint
🔇 Additional comments (35)
e2e/api/yaml-loader.ts (4)
50-50: LGTM! Good addition of valueType field.The addition of the optional
valueTypefield to support "direct" or "reference" values provides better type safety and clarity for deployment variable values.Also applies to: 114-114
128-168: Well-designed class structure for explicit entity creation order.The
EntitiesBuilderclass effectively addresses the PR objective by providing explicit control over entity creation order through separate methods. The constructor properly handles both new and existing entity scenarios, and the use of readonly properties ensures immutability of core data.
235-239: Excellent defensive programming with clear error messages.The validation check for system ID before creating environments prevents runtime errors and provides clear guidance to users about the required creation order.
212-218:Details
✅ Verification successful
Verify the expected status code for resource creation.
The status check expects 200 for resource creation, while other entity creations like system (line 181) and deployments (line 282) expect 201. Please verify this is correct according to your API specification.
🏁 Script executed:
#!/bin/bash # Search for other POST requests to /v1/resources to verify expected status codes rg -A 5 'POST.*"/v1/resources".*response\.status' --type tsLength of output: 61
To locate all usages and verify the expected status code, let’s search for every reference to
/v1/resourcesand howresourceResponseis handled:
🏁 Script executed:
#!/bin/bash # Locate any endpoint definitions or calls for /v1/resources rg -n '/v1/resources' --type ts # Find where resourceResponse is used to inspect status checks rg -n 'resourceResponse' --type ts # Find any .post(...) invocations related to resources rg -n '\.post.*resources' --type tsLength of output: 12001
Let’s inspect all POST calls and their corresponding status checks in
e2e/api/yaml-loader.tsto confirm which endpoints expect 200 vs. 201:#!/bin/bash # Show POST calls with a few lines of context to capture status checks rg -n 'this\.api\.POST' -A4 e2e/api/yaml-loader.ts
Resource creation endpoint correctly returns 200
Verified that all existing end-to-end tests for POST
/v1/resourcesassert a 200 status (e.g.e2e/tests/api/resources.spec.ts:299). The YAML loader’s check forstatus !== 200aligns with the API’s behavior—no change needed.e2e/tests/api/job-agents.spec.ts (1)
11-22: Proper implementation of the EntitiesBuilder pattern.The test setup correctly uses the new
EntitiesBuilderclass with appropriate entity creation order (system → environments → deployments) and includes proper cleanup. This is a good example of the refactored pattern.e2e/tests/api/release-targets.spec.ts (2)
4-22: LGTM! Clean refactor to builder pattern.The migration from
importEntitiesFromYamltoEntitiesBuilderis well-executed. The explicit stepwise creation methods (createSystem(),createResources(),createEnvironments(),createDeployments()) provide the desired control over entity creation order as outlined in the PR objectives.
24-73: Entity access properly updated to use builder pattern.All references to entities have been correctly updated to use
builder.resultinstead of the previousimportedEntitiesapproach. The test logic remains unchanged while benefiting from the improved entity management.e2e/tests/api/deployment-version.spec.ts (2)
5-21: Excellent refactor with consistent builder pattern implementation.The explicit creation sequence (
createSystem()→createDeployments()) allows for testing different entity creation orders. The refactor maintains test functionality while improving organization.
137-139: Good formatting improvement for error handling.Adding braces around single-line conditional throws improves code consistency and readability.
Also applies to: 180-182, 221-223
e2e/tests/api/deployments.spec.ts (2)
5-22: Comprehensive refactor demonstrates creation order flexibility.The explicit sequence (
createSystem()→createEnvironments()→createResources()) shows how the builder pattern enables different entity creation orders, fulfilling the PR's goal of explicit control over E2E test setup.
24-531: All entity references properly migrated to builder pattern.The systematic update of all
importedEntitiesreferences tobuilder.resultis thorough and maintains test functionality. The formatting improvements with braces enhance code consistency.e2e/tests/api/resources.spec.ts (3)
5-22: Consistent builder pattern completes comprehensive refactor.The creation sequence (
createSystem()→createEnvironments()→createDeployments()) maintains consistency with the other test files while providing the flexibility to test different entity creation orders.
284-286: Nice formatting improvement for multiline string interpolation.The formatting of the multiline string interpolation improves readability.
24-419: Entity management thoroughly migrated to builder pattern.All entity references have been systematically updated to use
builder.result, maintaining test functionality while benefiting from the improved entity management approach.e2e/tests/api/resource-relationships.spec.ts (2)
5-5: Excellent refactoring to the new EntitiesBuilder pattern!The migration from
importEntitiesFromYamltoEntitiesBuilderis well-executed and provides explicit control over entity creation order as intended. The setup now clearly separates system and resource creation steps.Also applies to: 11-11, 14-16, 20-20
24-27: Consistent entity access pattern updates.All references to
importedEntities.prefixhave been properly updated tobuilder.result.prefix. The variable naming and logic remain clear and functional.Also applies to: 37-40, 59-59, 72-72
e2e/tests/api/release.spec.ts (2)
5-5: Consistent EntitiesBuilder implementation.The refactoring follows the same systematic pattern with proper setup of system, resources, and environments. The explicit creation order provides better control over test entity dependencies.
Also applies to: 11-11, 14-17, 22-22
26-26: Proper entity access pattern migration.All references to the created entities have been correctly updated to use
builder.result, maintaining the same test logic while using the new infrastructure.Also applies to: 32-32, 48-48
e2e/tests/api/deployment-variable.spec.ts (2)
5-5: Well-executed EntitiesBuilder integration.The refactoring to use
EntitiesBuilderwith explicitcreateSystem()andcreateDeployments()calls provides clear control over the test setup process.Also applies to: 11-11, 14-16, 21-21
126-131: Improved formatting for variable value extraction.The reformatting of the ternary operators for variable value extraction improves readability while maintaining the same logic. The conditional handling of
valueType === "direct"is preserved correctly.Also applies to: 191-196, 200-202
e2e/tests/api/deployment-remove-event.spec.ts (2)
5-5: Appropriate selective use of EntitiesBuilder.The implementation correctly uses only
createSystem()since this test suite only requires system entities. This demonstrates the flexibility and efficiency of the new approach.Also applies to: 11-11, 14-15, 19-19
23-23: Consistent system entity access updates.All references to the system entity have been properly updated from
importedEntities.systemtobuilder.result.system. The system access pattern is consistent across all test cases.Also applies to: 105-105, 189-189, 274-274, 356-356
e2e/tests/api/environments.spec.ts (4)
5-5: LGTM! Import statement correctly updated for new EntitiesBuilder pattern.The import has been properly updated to include both
EntitiesBuilderandcleanupImportedEntitiesfrom the refactored API.
11-22: Excellent refactoring to EntitiesBuilder pattern.The migration from
importEntitiesFromYamlto the explicitEntitiesBuilderapproach is well-executed:
- Proper instantiation of
EntitiesBuilderwith required parameters- Explicit creation methods called in logical order:
createSystem()→createResources()→createDeployments()- Cleanup properly updated to use
builder.resultThis aligns perfectly with the PR objective of providing explicit control over entity creation order.
29-29: Entity references correctly updated to use builder pattern.All references to system entities have been properly migrated from the old imported entities to
builder.result.system, maintaining the same functionality while using the new API.Also applies to: 39-39, 43-43
378-385: Improved code formatting with proper brace usage.The formatting has been improved by adding braces around the single-line
ifstatement, which enhances code readability and follows JavaScript best practices.e2e/tests/api/yaml-import.spec.ts (4)
6-6: Import correctly updated for EntitiesBuilder pattern.The import statement has been properly updated to include
EntitiesBuilderalongside the existingcleanupImportedEntitiesfunction.
13-26: Comprehensive and well-structured entity creation process.This represents the most complete usage of the
EntitiesBuilderpattern in the codebase:
- Creates all entity types: system, resources, environments, deployments, deployment variables, and policies
- Follows logical creation order with proper dependencies
- Includes appropriate timing delay for resource processing
- Demonstrates the full capabilities of the new explicit creation approach
This perfectly showcases the PR's objective of providing granular control over entity creation order.
30-32: Good defensive programming with conditional cleanup check.The addition of the conditional check
if (builder.result)before cleanup is a good defensive programming practice, ensuring cleanup only occurs when entities were successfully created.
38-38: All entity references properly migrated to builder pattern.All references to imported entities have been consistently updated to use
builder.result, maintaining the same test logic while leveraging the new EntitiesBuilder API.Also applies to: 71-71, 74-74, 90-90, 92-92
e2e/tests/api/resource-variables.spec.ts (5)
5-5: Import statement correctly updated.The import has been properly updated to include both
EntitiesBuilderandcleanupImportedEntitiesfrom the refactored API.
14-18: Thoughtful selective entity creation for test requirements.The builder appropriately creates only the necessary baseline entities (
createSystem(),createEnvironments(),createDeployments()) while intentionally omittingcreateResources(). This is well-suited for this test suite since it creates resources dynamically within individual tests, demonstrating flexible usage of the EntitiesBuilder pattern.
21-21: Cleanup properly updated to use builder result.The cleanup logic has been correctly migrated to use
builder.resultwith thecleanupImportedEntitiesfunction.
25-25: Consistent entity reference updates across all test functions.All references to system entities have been systematically updated from the old imported entities to
builder.result.system, maintaining consistent access patterns throughout the test suite.Also applies to: 80-80, 138-138, 168-170, 261-263, 412-414, 586-588
320-320: Deployment references correctly updated.References to deployment entities have been properly migrated to use
builder.result.deployments[0], maintaining the same test logic while using the new builder pattern.Also applies to: 474-474, 647-647
| test.describe("Resource Provider API", () => { | ||
| let importedEntities: ImportedEntities; | ||
| let builder: EntitiesBuilder; | ||
| test.beforeAll(async ({ api, workspace }) => { | ||
| importedEntities = await importEntitiesFromYaml( | ||
| api, | ||
| workspace.id, | ||
| yamlPath, | ||
| ); | ||
| builder = new EntitiesBuilder(api, workspace, yamlPath); | ||
| await builder.createSystem(); | ||
| await builder.createResources(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding cleanup to prevent test data accumulation.
This test suite creates entities but doesn't clean them up in an afterAll hook, unlike other test files in this PR. This could lead to test data accumulation.
Add cleanup after the tests:
test.describe("Resource Provider API", () => {
let builder: EntitiesBuilder;
test.beforeAll(async ({ api, workspace }) => {
builder = new EntitiesBuilder(api, workspace, yamlPath);
await builder.createSystem();
await builder.createResources();
});
+
+ test.afterAll(async ({ api, workspace }) => {
+ await cleanupImportedEntities(api, builder.result, workspace.id);
+ });Don't forget to import cleanupImportedEntities:
-import { EntitiesBuilder } from "../../api";
+import { cleanupImportedEntities, EntitiesBuilder } from "../../api";📝 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.
| test.describe("Resource Provider API", () => { | |
| let importedEntities: ImportedEntities; | |
| let builder: EntitiesBuilder; | |
| test.beforeAll(async ({ api, workspace }) => { | |
| importedEntities = await importEntitiesFromYaml( | |
| api, | |
| workspace.id, | |
| yamlPath, | |
| ); | |
| builder = new EntitiesBuilder(api, workspace, yamlPath); | |
| await builder.createSystem(); | |
| await builder.createResources(); | |
| }); | |
| // Update import to include the cleanup helper | |
| -import { EntitiesBuilder } from "../../api"; | |
| +import { cleanupImportedEntities, EntitiesBuilder } from "../../api"; | |
| test.describe("Resource Provider API", () => { | |
| let builder: EntitiesBuilder; | |
| test.beforeAll(async ({ api, workspace }) => { | |
| builder = new EntitiesBuilder(api, workspace, yamlPath); | |
| await builder.createSystem(); | |
| await builder.createResources(); | |
| }); | |
| + // Ensure we clean up created entities after all tests run | |
| + test.afterAll(async ({ api, workspace }) => { | |
| + await cleanupImportedEntities(api, builder.result, workspace.id); | |
| + }); |
🤖 Prompt for AI Agents
In e2e/tests/api/resource-grouping.spec.ts around lines 9 to 16, the test suite
creates entities but lacks an afterAll hook to clean them up, which can cause
test data accumulation. Add an afterAll hook that calls cleanupImportedEntities
to remove the created entities after tests complete. Also, ensure you import
cleanupImportedEntities at the top of the file to enable this cleanup
functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
e2e/api/yaml-loader.ts (2)
195-228: Consider making error handling more consistent.The method checks for both existence and length of resources array, but some other methods only check for existence. Consider standardizing this pattern across all creation methods.
- if (!this.data.resources || this.data.resources.length === 0) { + if (!this.data.resources?.length) {This approach is more concise and handles both null/undefined and empty array cases consistently.
244-245: TODO comment indicates incomplete feature.The commented code suggests that resource selector handling is planned but not yet implemented. Consider tracking this as a separate task.
Would you like me to create an issue to track the resource selector implementation for environments?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
e2e/api/yaml-loader.ts(11 hunks)e2e/tests/api/deployment-remove-event.spec.ts(5 hunks)e2e/tests/api/deployment-variable.spec.ts(4 hunks)e2e/tests/api/deployment-version.spec.ts(11 hunks)e2e/tests/api/deployments.spec.ts(17 hunks)e2e/tests/api/environments.spec.ts(18 hunks)e2e/tests/api/job-agents.spec.ts(2 hunks)e2e/tests/api/release-targets.spec.ts(3 hunks)e2e/tests/api/release.spec.ts(11 hunks)e2e/tests/api/resource-relationships.spec.ts(13 hunks)e2e/tests/api/resource-variables.spec.ts(10 hunks)e2e/tests/api/resources.spec.ts(10 hunks)e2e/tests/api/yaml-import.spec.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- e2e/tests/api/job-agents.spec.ts
- e2e/tests/api/resource-variables.spec.ts
- e2e/tests/api/release.spec.ts
- e2e/tests/api/deployment-remove-event.spec.ts
- e2e/tests/api/deployment-variable.spec.ts
- e2e/tests/api/yaml-import.spec.ts
- e2e/tests/api/deployments.spec.ts
- e2e/tests/api/environments.spec.ts
- e2e/tests/api/deployment-version.spec.ts
- e2e/tests/api/release-targets.spec.ts
- e2e/tests/api/resource-relationships.spec.ts
- e2e/tests/api/resources.spec.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
e2e/api/yaml-loader.ts
🔇 Additional comments (7)
e2e/api/yaml-loader.ts (7)
50-50: LGTM: Added optional valueType field for better type safety.The addition of the optional
valueTypefield to both interfaces enhances type safety and aligns with the modular entity creation approach.Also applies to: 114-114
128-167: Well-structured constructor with proper initialization.The constructor properly handles file loading, templating with Handlebars, and initialization logic. The path resolution for both absolute and relative paths is handled correctly.
169-193: System creation method looks solid.The method properly validates the API response status (201) and updates the result object with the created system data including the original name for tracking.
235-239: Excellent dependency validation.The check for system ID before creating environments is a good defensive programming practice that ensures proper creation order dependencies.
312-317: Good defensive programming for deployment lookups.The methods properly validate that deployments exist in the result before adding versions/variables, preventing runtime errors.
Also applies to: 362-367
438-441: Updated function signature maintains backward compatibility.The cleanup function signature correctly uses the new
TestEntitiestype, maintaining consistency with the refactored builder pattern.
212-218:Details
❓ Verification inconclusive
Inconsistent HTTP status code expectations.
The creation methods expect different status codes for success:
- System: 201
- Resources: 200
- Environments: 200
- Deployments: 201
- Deployment versions: 201
- Deployment variables: 201
- Policies: 200
Please verify that these status code expectations align with your API design. If they should be consistent, consider standardizing them.
Also applies to: 254-258, 282-288, 330-336, 380-386, 418-422
🏁 Script executed:
#!/bin/bash # Search for API endpoint definitions to verify expected status codes rg -A 5 "POST.*/(systems|resources|environments|deployments|policies)" --type tsLength of output: 45314
🏁 Script executed:
#!/bin/bash # Extract status checks for each POST in yaml-loader.ts rg -n -C3 "systemResponse\.response\.status" -C3 e2e/api/yaml-loader.ts rg -n -C3 "resourceResponse\.response\.status" -C3 e2e/api/yaml-loader.ts rg -n -C3 "envResponse\.response\.status" -C3 e2e/api/yaml-loader.ts rg -n -C3 "deploymentResponse\.response\.status" -C3 e2e/api/yaml-loader.ts rg -n -C3 "variableResponse\.response\.status" -C3 e2e/api/yaml-loader.ts rg -n -C3 "policyResponse\.response\.status" -C3 e2e/api/yaml-loader.tsLength of output: 1786
Standardize Create Endpoint Status Codes
We’ve confirmed the following expectations in e2e/api/yaml-loader.ts:
- POST /v1/systems → expects 201 (line 181)
- POST /v1/resources → expects 200 (line 212)
- POST /v1/environments → expects 200 (line 254)
- POST /v1/deployments → expects 201 (line 282)
- POST /v1/deployments/{id}/variables → expects 201 (line 380)
- POST /v1/policies → expects 200 (line 418)
Please verify these align with your API specification. If all “create” calls should return the same status (e.g., 201), consider updating the 200 checks to 201 for consistency.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
e2e/tests/api/policies/matched-release-targets.spec.ts (1)
452-452:⚠️ Potential issueFix remaining references to undefined
importedEntitiesvariable.Multiple lines still reference the old
importedEntitiesvariable which is no longer defined after the migration toEntitiesBuilder. These will cause runtime errors.All references to
importedEntitiesshould be updated to usebuilder.cache:- const systemPrefix = importedEntities.system.slug.split("-")[0]!; + const systemPrefix = builder.cache.system.slug.split("-")[0]!; - systemId: importedEntities.system.id, + systemId: builder.cache.system.id,These changes need to be applied to lines: 452, 510, 586, 593, 664, 747, 853, 936, 1042, 1125, 1231, and 1359.
Also applies to: 510-510, 586-586, 593-593, 664-664, 747-747, 853-853, 936-936, 1042-1042, 1125-1125, 1231-1231, 1359-1359
🧹 Nitpick comments (2)
e2e/api/entities-builder.ts (2)
172-174: Address TODO for resource selector updates.There's an unimplemented TODO for updating resource selectors. This might be important for proper environment configuration.
Would you like me to help implement the resource selector update logic or create an issue to track this task?
366-414: Well-structured cleanup function with proper deletion order.The function correctly deletes entities in reverse dependency order and handles errors appropriately. Good use of optional chaining for defensive programming.
Consider making the error handling consistent by either throwing or logging for all deletion failures:
if (response.response.status !== 200) { - console.error( + throw new Error( `Failed to delete resource: ${JSON.stringify(response.error)}`, ); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
e2e/api/entities-builder.ts(1 hunks)e2e/api/entity-fixtures.ts(1 hunks)e2e/api/index.ts(1 hunks)e2e/api/schema.ts(3 hunks)e2e/tests/api/deployment-remove-event.spec.ts(5 hunks)e2e/tests/api/deployment-variable.spec.ts(4 hunks)e2e/tests/api/deployment-version.spec.ts(11 hunks)e2e/tests/api/deployments.spec.ts(17 hunks)e2e/tests/api/environments.spec.ts(17 hunks)e2e/tests/api/job-agents.spec.ts(2 hunks)e2e/tests/api/policies/matched-release-targets.spec.ts(7 hunks)e2e/tests/api/release-targets.spec.ts(3 hunks)e2e/tests/api/release.spec.ts(11 hunks)e2e/tests/api/resource-relationships.spec.ts(13 hunks)e2e/tests/api/resource-relationships.spec.yaml(3 hunks)e2e/tests/api/resource-variables.spec.ts(10 hunks)e2e/tests/api/resources.spec.ts(10 hunks)e2e/tests/api/workspace.spec.ts(0 hunks)e2e/tests/api/yaml-import.spec.ts(3 hunks)e2e/tests/api/yaml-import.spec.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- e2e/tests/api/workspace.spec.ts
✅ Files skipped from review due to trivial changes (3)
- e2e/api/index.ts
- e2e/tests/api/resource-relationships.spec.yaml
- e2e/tests/api/job-agents.spec.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- e2e/tests/api/release.spec.ts
- e2e/tests/api/yaml-import.spec.ts
- e2e/tests/api/deployment-version.spec.ts
- e2e/tests/api/release-targets.spec.ts
- e2e/tests/api/resources.spec.ts
- e2e/tests/api/resource-relationships.spec.ts
- e2e/tests/api/deployment-remove-event.spec.ts
- e2e/tests/api/deployment-variable.spec.ts
- e2e/tests/api/deployments.spec.ts
- e2e/tests/api/environments.spec.ts
- e2e/tests/api/resource-variables.spec.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
e2e/tests/api/policies/matched-release-targets.spec.tse2e/api/entities-builder.tse2e/api/entity-fixtures.tse2e/api/schema.ts
🧬 Code Graph Analysis (1)
e2e/tests/api/policies/matched-release-targets.spec.ts (1)
e2e/api/entities-builder.ts (2)
EntitiesBuilder(61-359)cleanupImportedEntities(366-414)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Lint
🔇 Additional comments (6)
e2e/api/entity-fixtures.ts (1)
1-130: Excellent implementation of entity fixture schemas and import functionality.This new module provides a robust foundation for entity fixture handling with:
- Comprehensive type-safe schemas using zod
- Proper path resolution for both absolute and relative paths
- Handlebars templating support with prefix parameter
- Good error handling using safeParse with descriptive error messages
- Clean separation of concerns between schema definition and import logic
e2e/tests/api/yaml-import.spec.yaml (1)
111-113: YAML structure change aligns with schema requirements.The modification of
versionAnyApprovalsfrom a single object to an array containing one object is consistent with thePolicyFixtureschema inentity-fixtures.ts, which defines this field asz.array(...). This change properly supports multiple approval configurations.e2e/api/schema.ts (2)
1-4: Auto-generated file changes look appropriate.These schema modifications align with the PR's goal of modularizing entity creation. The simplification from arrays to single objects suggests a more streamlined API contract.
1307-1307:Details
✅ Verification successful
Verify API contract changes are intentional.
The schema changes from arrays to single objects/simplified arrays should be validated against the source OpenAPI specification to ensure they're intentional and won't break existing integrations.
Run the following script to check if these schema changes are reflected in the OpenAPI spec file:
Also applies to: 1996-1996, 2724-2724, 2726-2726
🏁 Script executed:
#!/bin/bash # Description: Look for OpenAPI spec files and check for the schema definitions # Find OpenAPI spec files fd -e yaml -e yml -e json openapi api swagger spec | head -10 # Search for policy and deployment variable schema definitions rg -A 10 "versionAnyApprovals|createDeploymentVariable" --type yaml --type jsonLength of output: 2655
🏁 Script executed:
#!/bin/bash python3 << 'EOF' import json from collections import Counter # Load the OpenAPI spec with open("openapi.v1.json") as f: spec = json.load(f) # Print the component schema for VersionAnyApproval comp = spec.get("components", {}).get("schemas", {}).get("VersionAnyApproval") print("\ncomponents/schemas/VersionAnyApproval:") print(json.dumps(comp, indent=2)) # Traverse the spec to count occurrences of versionAnyApprovals counts = Counter() locations = [] def traverse(obj, path): if isinstance(obj, dict): for k, v in obj.items(): if k == "versionAnyApprovals": if isinstance(v, dict) and v.get("type") == "array": counts["array"] += 1 elif isinstance(v, dict) and v.get("$ref"): counts["ref"] += 1 else: counts["other"] += 1 locations.append((path + [k], v)) traverse(v, path + [k]) elif isinstance(obj, list): for idx, item in enumerate(obj): traverse(item, path + [str(idx)]) traverse(spec, []) print("\nOccurrence counts for 'versionAnyApprovals':") for key, cnt in counts.items(): print(f" {key}: {cnt}") print("\nDetailed locations for each occurrence:") for loc, v in locations: print(" Path:", "/".join(loc), "Definition:", "array" if v.get("type")=="array" else ("$ref" if v.get("$ref") else "other")) EOFLength of output: 657
API component schema matches the OpenAPI spec
I’ve verified that
components.schemas.VersionAnyApprovalis defined as an object (with a single requiredrequiredApprovalsCountproperty) inopenapi.v1.json, and that the two$refusages ofversionAnyApprovalsin the components match this. The lone inlinearraydefinition appears only in the PATCH request body (not in the shared schema), so the change ine2e/api/schema.tsto use the object type is intentional and correct—there is no breaking change to the component schema.No action required.
e2e/api/entities-builder.ts (2)
8-59: Well-structured cache interface.The
EntitiesCacheinterface provides a clear and comprehensive structure for tracking created entities with appropriate type definitions and optional properties.
72-92: Good constructor implementation with cache reuse support.The constructor properly handles both new entity creation and building from existing cache, which supports the PR's goal of allowing explicit control over entity creation order.
| builder.createSystem(); | ||
| builder.createResources(); | ||
| builder.createEnvironments(); | ||
| builder.createDeployments(); |
There was a problem hiding this comment.
Add missing await keywords for async method calls.
The entity creation methods are async but not being awaited, which will cause the entities to not be fully created before the tests run.
Apply this diff to fix the async calls:
- builder.createSystem();
- builder.createResources();
- builder.createEnvironments();
- builder.createDeployments();
+ await builder.createSystem();
+ await builder.createResources();
+ await builder.createEnvironments();
+ await builder.createDeployments();📝 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.
| builder.createSystem(); | |
| builder.createResources(); | |
| builder.createEnvironments(); | |
| builder.createDeployments(); | |
| await builder.createSystem(); | |
| await builder.createResources(); | |
| await builder.createEnvironments(); | |
| await builder.createDeployments(); |
🤖 Prompt for AI Agents
In e2e/tests/api/policies/matched-release-targets.spec.ts around lines 17 to 20,
the async methods builder.createSystem(), builder.createResources(),
builder.createEnvironments(), and builder.createDeployments() are called without
await, causing potential race conditions. Fix this by adding the await keyword
before each of these method calls to ensure they complete before proceeding with
the tests.
| body: { | ||
| ...resource, | ||
| workspaceId, | ||
| config: resource.config as Record<string, never>, |
There was a problem hiding this comment.
Incorrect type casting for resource config.
The type cast as Record<string, never> represents an empty object type that cannot have any properties. This seems incorrect for a config object that likely contains configuration data.
Consider using a more appropriate type:
- config: resource.config as Record<string, never>,
+ config: resource.config as Record<string, unknown>,📝 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.
| config: resource.config as Record<string, never>, | |
| config: resource.config as Record<string, unknown>, |
🤖 Prompt for AI Agents
In e2e/api/entities-builder.ts at line 133, the type cast 'as Record<string,
never>' is incorrect because it represents an empty object type, which is
unsuitable for a config object that should contain configuration data. Replace
this with a more appropriate type such as 'Record<string, any>' or a specific
interface/type that accurately reflects the structure of the config object to
ensure proper typing and avoid runtime issues.
| }, | ||
| ); | ||
|
|
||
| if (variableResponse.response.status !== 201) { |
There was a problem hiding this comment.
Incorrect status code check for deployment variable creation.
According to the OpenAPI schema, the createDeploymentVariable operation returns status 200 on success, not 201.
Update the status code check:
- if (variableResponse.response.status !== 201) {
+ if (variableResponse.response.status !== 200) {📝 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.
| if (variableResponse.response.status !== 201) { | |
| if (variableResponse.response.status !== 200) { |
🤖 Prompt for AI Agents
In e2e/api/entities-builder.ts at line 308, the status code check for the
createDeploymentVariable response incorrectly expects 201, but the OpenAPI
schema specifies 200 on success. Change the condition to check if
variableResponse.response.status is not equal to 200 instead of 201 to correctly
handle success responses.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
e2e/api/entities-builder.ts(1 hunks)e2e/api/entity-fixtures.ts(1 hunks)e2e/tests/api/deployment-remove-event.spec.ts(5 hunks)e2e/tests/api/jobs/job-flow-1.spec.ts(1 hunks)e2e/tests/api/jobs/job-flow-entities.spec.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- e2e/tests/api/jobs/job-flow-entities.spec.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- e2e/tests/api/deployment-remove-event.spec.ts
- e2e/api/entities-builder.ts
- e2e/api/entity-fixtures.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
e2e/tests/api/jobs/job-flow-1.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Lint
🔇 Additional comments (2)
e2e/tests/api/jobs/job-flow-1.spec.ts (2)
1-8: LGTM! Clean imports and path setup.The imports correctly use the new
EntitiesBuilderpattern and the path setup follows standard practices.
23-25: LGTM! Proper cleanup implementation.The cleanup using the builder's cache and workspace ID follows the new pattern correctly.
| test.beforeAll(async ({ api, workspace }) => { | ||
| builder = new EntitiesBuilder(api, workspace, yamlPath); | ||
| await builder.createSystem(); | ||
| await builder.createResources(); | ||
| await builder.createEnvironments(); | ||
| await builder.createDeployments(); | ||
| await builder.createAgents(); | ||
| await builder.createDeploymentVersions(); | ||
| await new Promise((resolve) => setTimeout(resolve, 5000)); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider replacing hardcoded delay with proper readiness checks.
The explicit entity creation order aligns well with the PR objectives. However, the hardcoded 5-second delay could lead to flaky tests.
Consider replacing the arbitrary timeout with proper readiness checks:
- await new Promise((resolve) => setTimeout(resolve, 5000));
+ // Wait for entities to be properly initialized
+ await builder.waitForReadiness();Alternatively, implement polling for specific conditions rather than a fixed delay.
📝 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.
| test.beforeAll(async ({ api, workspace }) => { | |
| builder = new EntitiesBuilder(api, workspace, yamlPath); | |
| await builder.createSystem(); | |
| await builder.createResources(); | |
| await builder.createEnvironments(); | |
| await builder.createDeployments(); | |
| await builder.createAgents(); | |
| await builder.createDeploymentVersions(); | |
| await new Promise((resolve) => setTimeout(resolve, 5000)); | |
| }); | |
| test.beforeAll(async ({ api, workspace }) => { | |
| builder = new EntitiesBuilder(api, workspace, yamlPath); | |
| await builder.createSystem(); | |
| await builder.createResources(); | |
| await builder.createEnvironments(); | |
| await builder.createDeployments(); | |
| await builder.createAgents(); | |
| await builder.createDeploymentVersions(); | |
| // Wait for entities to be properly initialized | |
| await builder.waitForReadiness(); | |
| }); |
🤖 Prompt for AI Agents
In e2e/tests/api/jobs/job-flow-1.spec.ts around lines 12 to 21, replace the
hardcoded 5-second delay after entity creation with a readiness check or polling
mechanism. Instead of waiting a fixed time, implement logic to verify that the
created entities or system components are fully ready or available before
proceeding, such as checking API responses or status flags in a loop with a
timeout. This will make the tests more reliable and reduce flakiness.
In order to test different orderings of entity creation, the yaml-entity loader:
Summary by CodeRabbit
Refactor
EntitiesBuilderclass.Chores
New Features
Bug Fixes