feat: force cleanup at boot with SYNC_FORCE_REMOVE=true
#956
Conversation
📝 WalkthroughWalkthroughThe pull request introduces a new optional configuration variable Changes
Sequence DiagramsequenceDiagram
participant Config as Configuration
participant Typegate as Typegate Instance
participant Engine as Registered Engines
Config->>Typegate: Initialize with forceRemove=true
Typegate->>Typegate: Check forceRemove flag
Typegate->>Engine: forceRemoveAllTypegraphs()
loop For each engine
Typegate->>Engine: removeTypegraph()
end
Typegate-->>Config: Log warning about typegraph removal
Possibly related PRs
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/typegate/src/main.ts (1)
56-59: LGTM! Consider adding more detailed logging.The implementation correctly handles the force removal of typegraphs at boot. However, it would be helpful to add more detailed logging about the number of typegraphs being removed.
if (config.sync?.forceRemove) { - logger.warn("Force removal of typegraphs at boot"); + const engines = typegate.register.list(); + logger.warn(`Force removal of ${engines.length} typegraphs at boot`); await typegate.forceRemoveAllTypegraphs(); }src/typegate/src/typegate/mod.ts (1)
400-404: Consider adding error handling for individual typegraph removals.While the implementation is correct, it could be more robust by handling failures of individual typegraph removals without stopping the entire process.
async forceRemoveAllTypegraphs() { + const errors: Array<[string, Error]> = []; for (const engine of this.register.list()) { - await this.removeTypegraph(engine.name); + try { + await this.removeTypegraph(engine.name); + } catch (error) { + logger.error(`Failed to remove typegraph ${engine.name}: ${error}`); + errors.push([engine.name, error]); + } } + if (errors.length > 0) { + throw new Error(`Failed to remove ${errors.length} typegraphs: ${errors.map(([name]) => name).join(', ')}`); + } }docs/metatype.dev/docs/reference/typegate/synchronization/index.mdx (1)
72-72: Enhance documentation with usage details and warnings.While the documentation is clear, it would be helpful to add more details about:
- The implications of using this feature
- When to use it
- Potential risks
-| SYNC\__FORCE_REMOVE (\_Optional_) | `true` or `false`, Undeploy cached typegraphs at boot | +| SYNC\__FORCE_REMOVE (\_Optional_) | `true` or `false`, Undeploy cached typegraphs at boot. Use with caution as this will remove all deployed typegraphs during startup, requiring redeployment. Useful for clean slate scenarios or recovering from corrupted states. |
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/metatype.dev/docs/reference/typegate/synchronization/index.mdx(1 hunks)src/typegate/src/config.ts(1 hunks)src/typegate/src/config/types.ts(1 hunks)src/typegate/src/main.ts(1 hunks)src/typegate/src/typegate/mod.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: bulid-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: test-full
- GitHub Check: pre-commit
🔇 Additional comments (3)
src/typegate/src/config/types.ts (1)
118-118: LGTM! Configuration schema changes are well-structured.The schema and type definitions for the force removal feature are correctly implemented with appropriate defaults and consistent naming.
Also applies to: 125-125
docs/metatype.dev/docs/reference/typegate/synchronization/index.mdx (1)
Line range hint
400-404: Add tests for the force removal feature.The feature would benefit from integration tests that verify:
- The force removal behavior when enabled/disabled
- Error handling during removal
- Interaction with system typegraphs
Would you like me to help create test cases for this feature?
src/typegate/src/config.ts (1)
120-120: LGTM! Clean implementation following existing conventions.The new property follows the established naming convention of transforming snake_case input to camelCase output, consistent with other properties in the configuration.
Let's verify the type definition update in the types file:
✅ Verification successful
Implementation verified - both type checking and runtime validation are in place
The
forceRemoveproperty is properly implemented with:
- Type definition in
SyncConfigX- Runtime validation through
syncConfigSchema- Consistent naming convention in the transformation layer
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the type definition update for SyncConfigX in config/types.ts # Test: Check if the forceRemove property is properly defined in SyncConfigX ast-grep --pattern 'type SyncConfigX = { $$$ forceRemove: $_ $$$ }' # Test: Check if force_remove is properly defined in syncConfigSchema ast-grep --pattern 'syncConfigSchema = $_.object({ $$$ force_remove: $_ $$$ })'Length of output: 511
Script:
#!/bin/bash # Search for zod schema definitions related to force_remove rg -A 5 "force_remove.*z\." || rg -A 5 "force_remove:.*boolean" # Search for any schema definition files fd "schema" --type f --exec rg -l "force_remove"Length of output: 111
Script:
#!/bin/bash # Find the schema validation pattern by looking at s3_bucket validation # since it's a known property in SyncConfigX rg -B 5 -A 5 "s3_bucket.*schema" || rg -B 5 -A 5 "s3_bucket:.*z\." # Look for the syncConfigSchema definition ast-grep --pattern 'syncConfigSchema = $_.object({ $$$ })'Length of output: 130
Script:
#!/bin/bash # Look at the config.ts file content to understand the validation pattern cat src/typegate/src/config.tsLength of output: 4046
|
@coderabbitai can you generate tests for this functionality in a new file within |
|
Tip For best results, initiate chat on the files or code changes. @Yohe-Am Here's a test file for the force cleanup functionality. I suggest creating it as // Copyright Metatype OÜ, licensed under the Mozilla Public License Version 2.0.
// SPDX-License-Identifier: MPL-2.0
import { Meta } from "test-utils/mod.ts";
import { connect } from "redis";
import { S3Client } from "aws-sdk/client-s3";
import { createBucket, listObjects, tryDeleteBucket } from "test-utils/s3.ts";
import { assertEquals } from "@std/assert";
const redisKey = "typegraph";
const redisEventKey = "typegraph_event";
async function cleanUp(config: typeof syncConfig) {
using redis = await connect(config.redis);
await redis.del(redisKey);
await redis.del(redisEventKey);
const s3 = new S3Client(config.s3);
await tryDeleteBucket(s3, config.s3Bucket);
await createBucket(s3, config.s3Bucket);
s3.destroy();
await redis.quit();
}
const syncConfig = {
redis: {
hostname: "localhost",
port: 6379,
password: "password",
db: 4,
},
s3: {
endpoint: "http://localhost:9000",
region: "local",
credentials: {
accessKeyId: "minio",
secretAccessKey: "password",
},
forcePathStyle: true,
},
s3Bucket: "metatype-force-cleanup-test",
force_remove: false,
};
Meta.test(
{
name: "Force Cleanup: Sync Config",
syncConfig,
async setup() {
await cleanUp(syncConfig);
},
async teardown() {
await cleanUp(syncConfig);
},
},
async (metaTest) => {
await metaTest.should("preserve typegraphs when force_remove is false", async () => {
// Deploy first engine
const engine1 = await metaTest.engine("runtimes/wasm_wire/wasm_wire.ts");
// Create new typegate instance without force_remove
const engine2 = await metaTest.engine("runtimes/wasm_wire/wasm_wire.ts");
// Verify both engines are still available
const s3 = new S3Client(syncConfig.s3);
const objects = await listObjects(s3, syncConfig.s3Bucket);
assertEquals(objects?.length, 3); // artifact + typegraphs
s3.destroy();
});
await metaTest.should("remove typegraphs when force_remove is true", async () => {
// Update config to enable force_remove
const forceConfig = {
...syncConfig,
force_remove: true,
};
// Deploy first engine
const engine1 = await metaTest.engine("runtimes/wasm_wire/wasm_wire.ts");
// Create new typegate instance with force_remove enabled
const engine2 = await metaTest.engine("runtimes/wasm_wire/wasm_wire.ts", {
syncConfig: forceConfig,
});
// Verify typegraphs were removed
const s3 = new S3Client(syncConfig.s3);
const objects = await listObjects(s3, syncConfig.s3Bucket);
assertEquals(objects?.length, 0); // All typegraphs should be removed
s3.destroy();
});
},
);This test file:
Let me know if you'd like any modifications to the test cases! 🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 3372 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/typegate/src/sync/replicated_map.ts (1)
108-120: Consider using array destructuring for better readability.The implementation is correct, but readability could be improved by using array destructuring to handle key-value pairs.
- for (let i = 0; i < all.length; i += 2) { - history.push({ - name: all[i], - payload: all[i+1] - }); + for (let i = 0; i < all.length; i += 2) { + const [name, payload] = [all[i], all[i+1]]; + history.push({ name, payload }); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/typegate/src/main.ts(0 hunks)src/typegate/src/sync/replicated_map.ts(1 hunks)src/typegate/src/typegate/mod.ts(3 hunks)src/typegate/src/typegate/register.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/typegate/src/main.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: bulid-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: test-full
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: pre-commit
🔇 Additional comments (3)
src/typegate/src/typegate/register.ts (1)
89-89: LGTM! Constructor parameter visibility change is appropriate.The change from private to public visibility for
replicatedMapis necessary to support the new force cleanup feature by allowing access to the map's history.src/typegate/src/sync/replicated_map.ts (1)
133-141: LGTM! Proper error handling and logging.The implementation includes appropriate error handling with the
ensurecheck and informative logging.src/typegate/src/typegate/mod.ts (1)
410-424: LGTM! Comprehensive force cleanup implementation.The
forceRemovemethod properly handles:
- Typegraph ID parsing
- Artifact reference cleanup
- Garbage collection
- Logging for observability
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/typegate/src/typegate/mod.ts (1)
415-429: Add explicit error handling for download and schema validation.While the implementation is functionally correct, consider enhancing error handling:
async forceRemove(name: string, payload: string, typegraphStore: TypegraphStore) { logger.warn(`Dropping "${name}": started`); - const typegraphId = typegraphIdSchema.parse(JSON.parse(payload)); - const [tg] = await typegraphStore.download( - typegraphId, - ); + let typegraphId; + try { + typegraphId = typegraphIdSchema.parse(JSON.parse(payload)); + } catch (e) { + throw new Error(`Invalid typegraph ID in payload for "${name}": ${e.message}`); + } + + let tg; + try { + [tg] = await typegraphStore.download(typegraphId); + } catch (e) { + throw new Error(`Failed to download typegraph "${name}": ${e.message}`); + } const artifacts = new Set( Object.values(tg.meta.artifacts).map((m) => m.hash), ); await this.register.remove(name); await this.artifactStore.updateRefCounts(new Set(), artifacts); await this.artifactStore.runArtifactGC(); logger.warn(`Dropping "${name}": done`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/typegate/src/config/types.ts(1 hunks)src/typegate/src/typegate/mod.ts(3 hunks)tests/sync/sync_config_test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/typegate/src/config/types.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: bulid-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: test-full
- GitHub Check: pre-commit
🔇 Additional comments (4)
tests/sync/sync_config_test.ts (1)
45-45: LGTM!The test case correctly verifies that
forceRemovedefaults tofalsewhen sync is enabled.src/typegate/src/typegate/mod.ts (3)
46-46: LGTM!The import statement correctly includes the required dependencies for the new force cleanup functionality.
144-148: LGTM!The initialization sequence correctly integrates the TypegraphStore with the ReplicatedRegister.
155-166: LGTM! Robust implementation with proper error handling.The force cleanup implementation correctly:
- Checks the configuration flag
- Safely iterates through history
- Handles errors gracefully with proper logging and Sentry integration
- Continues processing even if individual removals fail
@coderabbitai this test does not reflect the new changes, can you review one last time now that engines are not produced anymore from the redis and s3? We drop all of it before sync (note: sync creates engines from stored typegraphs). |
|
Tip For best results, initiate chat on the files or code changes. @michael-0acf4 Here's the updated test file that correctly reflects the new changes: // Copyright Metatype OÜ, licensed under the Mozilla Public License Version 2.0.
// SPDX-License-Identifier: MPL-2.0
import { Meta } from "test-utils/mod.ts";
import { connect } from "redis";
import { S3Client } from "aws-sdk/client-s3";
import { createBucket, listObjects, tryDeleteBucket } from "test-utils/s3.ts";
import { assertEquals } from "@std/assert";
import { Typegate } from "@metatype/typegate/typegate/mod.ts";
const redisKey = "typegraph";
const redisEventKey = "typegraph_event";
async function cleanUp(config: typeof syncConfig) {
using redis = await connect(config.redis);
await redis.del(redisKey);
await redis.del(redisEventKey);
const s3 = new S3Client(config.s3);
await tryDeleteBucket(s3, config.s3Bucket);
await createBucket(s3, config.s3Bucket);
s3.destroy();
await redis.quit();
}
const syncConfig = {
redis: {
hostname: "localhost",
port: 6379,
password: "password",
db: 4,
},
s3: {
endpoint: "http://localhost:9000",
region: "local",
credentials: {
accessKeyId: "minio",
secretAccessKey: "password",
},
forcePathStyle: true,
},
s3Bucket: "metatype-force-cleanup-test",
force_remove: false,
};
Meta.test(
{
name: "Force Cleanup: Sync Config",
syncConfig,
async setup() {
await cleanUp(syncConfig);
},
async teardown() {
await cleanUp(syncConfig);
},
},
async (metaTest) => {
await metaTest.should("preserve stored typegraphs when force_remove is false", async () => {
// Deploy typegraphs to store them in Redis and S3
await metaTest.engine("runtimes/wasm_wire/wasm_wire.ts");
// Verify typegraphs are stored
const s3 = new S3Client(syncConfig.s3);
const objects = await listObjects(s3, syncConfig.s3Bucket);
assertEquals(objects?.length, 3); // artifact + typegraphs
// Create new typegate instance without force_remove
const typegate2 = await Typegate.init(syncConfig);
// Verify typegraphs are preserved
const objectsAfterSync = await listObjects(s3, syncConfig.s3Bucket);
assertEquals(objectsAfterSync?.length, 3);
await typegate2.deinit();
s3.destroy();
});
await metaTest.should("remove stored typegraphs when force_remove is true", async () => {
// Deploy typegraphs to store them in Redis and S3
await metaTest.engine("runtimes/wasm_wire/wasm_wire.ts");
// Verify typegraphs are stored
const s3 = new S3Client(syncConfig.s3);
const objects = await listObjects(s3, syncConfig.s3Bucket);
assertEquals(objects?.length, 3);
// Create new typegate instance with force_remove enabled
const forceConfig = {
...syncConfig,
force_remove: true,
};
const typegate2 = await Typegate.init(forceConfig);
// Verify typegraphs are removed
const objectsAfterSync = await listObjects(s3, syncConfig.s3Bucket);
assertEquals(objectsAfterSync?.length, 0);
await typegate2.deinit();
s3.destroy();
});
},
);Key changes made:
Let me know if you'd like any adjustments to the test cases! 🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 2665 Script: Length of output: 6201 ✏️ Learnings added
|
|
Tip For best results, initiate chat on the files or code changes. @michael-0acf4 Here's the updated test file that reflects the new sync behavior: // Copyright Metatype OÜ, licensed under the Mozilla Public License Version 2.0.
// SPDX-License-Identifier: MPL-2.0
import { gql, Meta } from "test-utils/mod.ts";
import { connect } from "redis";
import { S3Client } from "aws-sdk/client-s3";
import { createBucket, listObjects, tryDeleteBucket } from "test-utils/s3.ts";
import { assertEquals } from "@std/assert";
const redisKey = "typegraph";
const redisEventKey = "typegraph_event";
async function cleanUp(config: typeof syncConfig) {
using redis = await connect(config.redis);
await redis.del(redisKey);
await redis.del(redisEventKey);
const s3 = new S3Client(config.s3);
await tryDeleteBucket(s3, config.s3Bucket);
await createBucket(s3, config.s3Bucket);
s3.destroy();
await redis.quit();
}
const syncConfig = {
redis: {
hostname: "localhost",
port: 6379,
password: "password",
db: 4,
},
s3: {
endpoint: "http://localhost:9000",
region: "local",
credentials: {
accessKeyId: "minio",
secretAccessKey: "password",
},
forcePathStyle: true,
},
s3Bucket: "metatype-force-cleanup-test",
force_remove: false,
};
Meta.test(
{
name: "Force Cleanup: Sync Config",
syncConfig,
async setup() {
await cleanUp(syncConfig);
},
async teardown() {
await cleanUp(syncConfig);
},
},
async (metaTest) => {
await metaTest.should("preserve typegraphs during sync when force_remove is false", async () => {
// Store initial typegraph
const engine1 = await metaTest.engine("runtimes/wasm_wire/wasm_wire.ts");
// Verify initial state
const s3 = new S3Client(syncConfig.s3);
const initialObjects = await listObjects(s3, syncConfig.s3Bucket);
assertEquals(initialObjects?.length, 3); // artifact + typegraphs
// Create new typegate instance without force_remove
const engine2 = await metaTest.engine("runtimes/wasm_wire/wasm_wire.ts");
// Verify typegraphs are preserved and functional
await gql`
query {
add(a: 11, b: 2)
}
`
.expectData({
add: 13,
})
.on(engine2);
const finalObjects = await listObjects(s3, syncConfig.s3Bucket);
assertEquals(finalObjects?.length, 3); // Should remain the same
s3.destroy();
});
await metaTest.should("remove typegraphs during sync when force_remove is true", async () => {
// Store initial typegraph
const engine1 = await metaTest.engine("runtimes/wasm_wire/wasm_wire.ts");
// Verify initial state
const s3 = new S3Client(syncConfig.s3);
const initialObjects = await listObjects(s3, syncConfig.s3Bucket);
assertEquals(initialObjects?.length, 3);
// Create new typegate instance with force_remove enabled
const forceConfig = {
...syncConfig,
force_remove: true,
};
const engine2 = await metaTest.engine("runtimes/wasm_wire/wasm_wire.ts", {
syncConfig: forceConfig,
});
// Verify typegraphs are removed during sync
const intermediateObjects = await listObjects(s3, syncConfig.s3Bucket);
assertEquals(intermediateObjects?.length, 0);
// Verify new typegraph is created and functional after sync
await gql`
query {
add(a: 11, b: 2)
}
`
.expectData({
add: 13,
})
.on(engine2);
const finalObjects = await listObjects(s3, syncConfig.s3Bucket);
assertEquals(finalObjects?.length, 3); // New typegraph should be created
s3.destroy();
});
},
);Key changes made:
Let me know if you'd like any adjustments to the test cases! 🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 3331 |
|
Tip For best results, initiate chat on the files or code changes. @michael-0acf4 Here's the updated test file that correctly reflects the sync behavior: // Copyright Metatype OÜ, licensed under the Mozilla Public License Version 2.0.
// SPDX-License-Identifier: MPL-2.0
import { gql, Meta } from "test-utils/mod.ts";
import { connect } from "redis";
import { S3Client } from "aws-sdk/client-s3";
import { createBucket, listObjects, tryDeleteBucket } from "test-utils/s3.ts";
import { assertEquals } from "@std/assert";
const redisKey = "typegraph";
const redisEventKey = "typegraph_event";
async function cleanUp(config: typeof syncConfig) {
using redis = await connect(config.redis);
await redis.del(redisKey);
await redis.del(redisEventKey);
const s3 = new S3Client(config.s3);
await tryDeleteBucket(s3, config.s3Bucket);
await createBucket(s3, config.s3Bucket);
s3.destroy();
await redis.quit();
}
const syncConfig = {
redis: {
hostname: "localhost",
port: 6379,
password: "password",
db: 4,
},
s3: {
endpoint: "http://localhost:9000",
region: "local",
credentials: {
accessKeyId: "minio",
secretAccessKey: "password",
},
forcePathStyle: true,
},
s3Bucket: "metatype-force-cleanup-test",
force_remove: false,
};
Meta.test(
{
name: "Force Cleanup: Sync Config",
syncConfig,
async setup() {
await cleanUp(syncConfig);
},
async teardown() {
await cleanUp(syncConfig);
},
},
async (metaTest) => {
await metaTest.should("preserve stored typegraphs during sync when force_remove is false", async () => {
// Store initial typegraph
const engine1 = await metaTest.engine("runtimes/wasm_wire/wasm_wire.ts");
// Verify initial storage state
const s3 = new S3Client(syncConfig.s3);
const initialObjects = await listObjects(s3, syncConfig.s3Bucket);
assertEquals(initialObjects?.length, 3); // artifact + typegraphs
// Create new typegate instance without force_remove
const engine2 = await metaTest.engine("runtimes/wasm_wire/wasm_wire.ts");
// Verify typegraphs are preserved and functional after sync
await gql`
query {
add(a: 11, b: 2)
}
`
.expectData({
add: 13,
})
.on(engine2);
const finalObjects = await listObjects(s3, syncConfig.s3Bucket);
assertEquals(finalObjects?.length, 3); // Should remain the same
s3.destroy();
});
await metaTest.should("remove stored typegraphs during sync when force_remove is true", async () => {
// Store initial typegraph
const engine1 = await metaTest.engine("runtimes/wasm_wire/wasm_wire.ts");
// Verify initial storage state
const s3 = new S3Client(syncConfig.s3);
const initialObjects = await listObjects(s3, syncConfig.s3Bucket);
assertEquals(initialObjects?.length, 3);
// Create new typegate instance with force_remove enabled
const forceConfig = {
...syncConfig,
force_remove: true,
};
const engine2 = await metaTest.engine("runtimes/wasm_wire/wasm_wire.ts", {
syncConfig: forceConfig,
});
// Verify typegraphs are removed during sync
const intermediateObjects = await listObjects(s3, syncConfig.s3Bucket);
assertEquals(intermediateObjects?.length, 0);
// Verify new typegraph is created and functional after sync
await gql`
query {
add(a: 11, b: 2)
}
`
.expectData({
add: 13,
})
.on(engine2);
const finalObjects = await listObjects(s3, syncConfig.s3Bucket);
assertEquals(finalObjects?.length, 3); // New typegraph should be created
s3.destroy();
});
},
);Key changes:
Let me know if you'd like any adjustments to the test cases! 🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 3331 ✏️ Learnings added
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
tests/e2e/published/published_test.ts (1)
Line range hint
19-31: Add SYNC_FORCE_REMOVE to environment variables.The
syncEnvsobject should include the newSYNC_FORCE_REMOVEenvironment variable to ensure consistent configuration across the test process.Apply this diff:
const syncEnvs = { SYNC_REDIS_URL: "redis://:password@localhost:6379/12", SYNC_S3_HOST: "http://localhost:9000", SYNC_S3_REGION: "local", SYNC_S3_ACCESS_KEY: "minio", SYNC_S3_SECRET_KEY: "password", SYNC_S3_BUCKET: "upgrade-test", SYNC_S3_PATH_STYLE: "true", + SYNC_FORCE_REMOVE: "false", };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/e2e/published/published_test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: test-full
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: bulid-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: pre-commit
🔇 Additional comments (1)
tests/e2e/published/published_test.ts (1)
Line range hint
186-187: Add test cases for force cleanup functionality.The test suite verifies upgrade paths and SDK compatibility but lacks explicit tests for the new force cleanup feature. Consider adding test cases that:
- Verify typegraphs are preserved when
force_removeis false- Verify typegraphs are cleaned up when
force_removeis true- Test the behavior during typegate restart
This aligns with the user's request for sync-related tests in the PR comments.
Here's a verification script to check if we have any existing force cleanup tests:
Also applies to: 234-235
✅ Verification successful
Add test cases for force cleanup functionality
The verification confirms that while
force_removeconfiguration exists in the test suite, there are no explicit test cases verifying its behavior. The current tests only useforce_remove: falsewithout testing the cleanup functionality when enabled.
- Add test variations in
tests/e2e/published/published_test.tsto verify:
- Directory preservation with
force_remove: false- Directory cleanup with
force_remove: true- Cleanup behavior during typegate restart
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for existing force cleanup tests # Test: Search for test files and test cases related to force cleanup rg -l 'force.*cleanup|force.*remove' tests/sync/Length of output: 50
Script:
#!/bin/bash # Search for force cleanup related tests across all test directories rg -l "force.*(?:clean|remov|delet)" tests/ rg -C 3 "removeIfExists|cleanupDir|force" tests/e2e/published/published_test.tsLength of output: 704
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
tests/sync/sync_force_remove_test.ts (6)
20-30: Add error handling to cleanup operations.The cleanup function should handle potential failures gracefully. Consider wrapping Redis and S3 operations in try-catch blocks.
async function cleanUp(config: typeof syncConfig) { - using redis = await connect(config.redis); - await redis.del(redisKey); - await redis.del(redisEventKey); + try { + using redis = await connect(config.redis); + await redis.del(redisKey); + await redis.del(redisEventKey); + await redis.quit(); + } catch (error) { + console.error('Failed to cleanup Redis:', error); + } - const s3 = new S3Client(config.s3); - await tryDeleteBucket(s3, config.s3Bucket); - await createBucket(s3, config.s3Bucket); - s3.destroy(); - await redis.quit(); + try { + const s3 = new S3Client(config.s3); + await tryDeleteBucket(s3, config.s3Bucket); + await createBucket(s3, config.s3Bucket); + s3.destroy(); + } catch (error) { + console.error('Failed to cleanup S3:', error); + } }
32-49: Document test credentials and configuration.Consider adding a comment explaining that these are test-only credentials and configuration. This helps prevent confusion and misuse in production environments.
+// Test-only configuration. These credentials are for local development and testing. +// Do not use these values in production environments. const syncConfig = { redis: { hostname: "localhost",
51-62: Add JSDoc documentation and return type.Consider adding JSDoc documentation and explicit return type for better code maintainability and type safety.
+/** + * Creates and initializes a new Typegate instance with the provided sync configuration. + * @param syncConfig - The synchronization configuration to use + * @returns A Promise that resolves to an initialized Typegate instance + */ -async function spawnGate(syncConfig: SyncConfig) { +async function spawnGate(syncConfig: SyncConfig): Promise<Typegate> {
89-89: Document the expected object count.The assertion expects exactly 3 objects in S3. Add a comment explaining what these objects represent for better test maintainability.
- assertEquals(initialObjects?.length, 3); + // Expect 3 objects: typegraph definition, runtime configuration, and metadata + assertEquals(initialObjects?.length, 3);
110-110: Replace cryptic comment with descriptive one.The "// !" comment is not descriptive. Replace it with a meaningful comment explaining the significance of empty names array.
- assertEquals(namesAfterRemove, []); // ! + // Verify that all typegraphs were removed after force cleanup + assertEquals(namesAfterRemove, []);
64-114: Add negative test cases.Consider adding test cases for:
- Invalid sync configurations
- Network failures during cleanup
- Partial cleanup scenarios
This would improve the robustness of the test suite.
Would you like me to help generate these additional test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/sync/scripts/hello.ts(1 hunks)tests/sync/sync.py(1 hunks)tests/sync/sync_force_remove_test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: test-full
- GitHub Check: bulid-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: pre-commit
🔇 Additional comments (6)
tests/sync/sync_force_remove_test.ts (1)
1-19: LGTM! Well-structured imports and constants.The file has proper headers, necessary imports, and well-defined constants.
tests/sync/scripts/hello.ts (2)
1-2: LGTM! Proper license headers included.The file includes the correct Mozilla Public License Version 2.0 headers.
4-6: LGTM! Clean and type-safe implementation.The function implementation is concise and properly typed using TypeScript's object destructuring pattern.
tests/sync/sync.py (3)
1-2: LGTM! Proper license headers included.The file includes the correct Mozilla Public License Version 2.0 headers.
4-6: LGTM! Required imports are present.All necessary imports are included for the typegraph implementation.
8-21: Verify secret configuration and module path.The implementation looks good, but there are two items that need verification:
- The secret "ULTRA_SECRET" is referenced but needs to be configured in the environment.
- The relative module path "scripts/hello.ts" needs to be verified.
Let's verify the module path resolution and find any related secret configurations:
✅ Verification successful
Module path and secret configuration are correctly set up ✅
The verification shows that:
- The module path "scripts/hello.ts" correctly resolves to an existing implementation
- The ULTRA_SECRET is properly configured for test purposes, being injected during test execution
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the module path is correct relative to the test directory fd hello.ts tests/ # Look for any environment variable configurations or secret definitions rg -i "ULTRA_SECRET" --type yaml --type typescript --type pythonLength of output: 200
Script:
#!/bin/bash # Check the content of the hello.ts file cat tests/sync/scripts/hello.ts # Search for secret configuration without type restrictions rg -i "ULTRA_SECRET" # Look for environment/secret configurations in common locations fd -e yaml -e yml -e envLength of output: 1332
Script:
#!/bin/bash # Check the test file that references the secret cat tests/sync/sync_force_remove_test.ts # Check content of test configuration files cat tests/metatype.ymlLength of output: 4251
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #956 +/- ##
==========================================
+ Coverage 77.72% 77.97% +0.24%
==========================================
Files 154 154
Lines 19154 19057 -97
Branches 1930 1923 -7
==========================================
- Hits 14888 14859 -29
+ Misses 4243 4175 -68
Partials 23 23 ☔ View full report in Codecov by Sentry. |
Migration notes
None
Summary by CodeRabbit
New Features
SYNC_FORCE_REMOVEconfiguration variable for typegate synchronization.Documentation
Improvements
Typegateclass with a method to facilitate bulk removal of typegraphs during initialization.replicatedMapparameter publicly accessible in theReplicatedRegisterclass constructor.forceRemoveproperty.