init eph envs #179
init eph envs #179
Conversation
|
Warning Rate limit exceeded@adityachoudhari26 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 19 seconds before requesting another review. ⌛ 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. WalkthroughThe pull request introduces significant modifications to the database schema in the Changes
Possibly related PRs
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
packages/db/src/schema/release.ts (1)
51-51: Add documentation for the releaseChannel table.Since this table is part of the ephemeral environments feature, consider adding a comment block above the table definition explaining:
- The purpose of release channels
- How they relate to ephemeral environments
- The significance of the releaseFilter field
+/** + * Release channels manage the flow of releases to environments, + * particularly for ephemeral environment management. + * + * The releaseFilter determines which releases can flow through + * this channel based on conditions like metadata, version, or + * creation date. + */ export const releaseChannel = pgTable("release_channel", {packages/db/src/schema/environment.ts (2)
96-100: Consider including units in the field name for clarityWhile the comment specifies that
rolloutDurationis in milliseconds, explicitly indicating the unit in the field name (e.g.,rolloutDurationMs) can enhance clarity and prevent potential confusion.
102-105: Include units inephemeralDurationfield name for consistencySimilar to
rolloutDuration, consider renamingephemeralDurationtoephemeralDurationMsto make the unit of measurement explicit and maintain consistency.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- packages/db/src/schema/environment.ts (5 hunks)
- packages/db/src/schema/release.ts (1 hunks)
🔇 Additional comments (4)
packages/db/src/schema/release.ts (1)
58-60: Verify releaseFilter compatibility with existing condition builders.The
releaseFilterfield uses the sameReleaseConditiontype that's used by thebuildConditionfunction. Ensure that thereleaseMatchesConditionfunction (lines 257-262) properly handles the new use case from the release channel.✅ Verification successful
Release filter implementation is compatible with existing usage
Based on the verification results:
- The
releaseMatchesConditionfunction is used consistently across the codebase in multiple components (job-dispatch, API routes) with the sameReleaseConditiontype- The
ReleaseConditiontype is well-defined and shared across the codebase- The new
releaseFilterfield in the release channel table follows the same pattern as existing implementations in other components- Existing code already handles null cases for release filters
The implementation is consistent with current usage patterns and type definitions, ensuring compatibility with the condition building functions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any existing usage of releaseMatchesCondition with release channels # to ensure compatibility # Look for any connections between release channels and condition matching rg -l "releaseMatchesCondition.*releaseChannel|releaseChannel.*releaseMatchesCondition" # Check for any potential type mismatches or incompatible usage ast-grep --pattern 'releaseMatchesCondition($_, $release_filter)'Length of output: 157
Script:
#!/bin/bash # Let's first check the implementation of releaseMatchesCondition and its usage rg -A 5 "releaseMatchesCondition" # Check the ReleaseCondition type definition rg -A 5 "type ReleaseCondition" # Look for any existing usage of releaseFilter rg "releaseFilter" # Check for any release channel related functions rg -A 5 "releaseChannel"Length of output: 11384
packages/db/src/schema/environment.ts (3)
21-22: Imports correctly added for new dependenciesThe new imports for
deployment,release, andreleaseChannelare necessary for the entities used later in the schema definitions.
41-43: Good practice addingcreatedAttimestampIncluding the
createdAtfield with a default of the current time ensures that the creation time of each environment is recorded, which is beneficial for tracking and auditing purposes.
114-116: Ensure validation for new duration fields increateEnvironmentPolicyWith the addition of
rolloutDurationandephemeralDuration, please ensure thatcreateEnvironmentPolicyincludes appropriate validation for these fields. This could involve setting minimum and maximum acceptable values to prevent invalid or unintended data inputs.
| export const releaseChannel = pgTable("release_channel", { | ||
| id: uuid("id").primaryKey().defaultRandom(), | ||
| name: text("name").notNull(), | ||
| description: text("description").default(""), | ||
| deploymentId: uuid("deployment_id") | ||
| .notNull() | ||
| .references(() => deployment.id, { onDelete: "cascade" }), | ||
| releaseFilter: jsonb("release_filter") | ||
| .$type<ReleaseCondition | null>() | ||
| .default(sql`NULL`), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding constraints and audit fields to the releaseChannel table.
While the basic structure is sound, consider the following improvements:
- Add a unique constraint to prevent duplicate channel names within a deployment:
export const releaseChannel = pgTable("release_channel", {
id: uuid("id").primaryKey().defaultRandom(),
name: text("name").notNull(),
description: text("description").default(""),
deploymentId: uuid("deployment_id")
.notNull()
.references(() => deployment.id, { onDelete: "cascade" }),
releaseFilter: jsonb("release_filter")
.$type<ReleaseCondition | null>()
.default(sql`NULL`),
+ }, (t) => ({
+ unq: uniqueIndex().on(t.name, t.deploymentId)
});- Add timestamp fields for better auditing:
releaseFilter: jsonb("release_filter")
.$type<ReleaseCondition | null>()
.default(sql`NULL`),
+ createdAt: timestamp("created_at").notNull().defaultNow(),
+ updatedAt: timestamp("updated_at").notNull().defaultNow(),- Consider using NULL instead of empty string for description:
- description: text("description").default(""),
+ description: text("description").default(sql`NULL`),Committable suggestion was skipped due to low confidence.
| export const environmentReleaseChannel = pgTable( | ||
| "environment_release_channel", | ||
| { | ||
| id: uuid("id").primaryKey().defaultRandom(), | ||
| environmentId: uuid("environment_id") | ||
| .notNull() | ||
| .references(() => environment.id, { onDelete: "cascade" }), | ||
| channelId: uuid("channel_id") | ||
| .notNull() | ||
| .references(() => releaseChannel.id, { onDelete: "cascade" }), | ||
| }, | ||
| (t) => ({ | ||
| uniq: uniqueIndex().on(t.environmentId, t.channelId), | ||
| deploymentUniq: uniqueIndex().on( | ||
| t.environmentId, | ||
| sql`(SELECT ${deployment.id} FROM ${releaseChannel} WHERE id = ${t.channelId})`, | ||
| ), | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Potential issue with unique index using a subquery
Similar to the previous comment, the deploymentUniq unique index in the environmentReleaseChannel table uses a subquery, which may not be valid in index definitions and could cause database errors.
Consider modifying the schema to avoid subqueries in the unique index. Adding a deploymentId field and adjusting the unique index accordingly can resolve this issue:
export const environmentReleaseChannel = pgTable(
"environment_release_channel",
{
id: uuid("id").primaryKey().defaultRandom(),
environmentId: uuid("environment_id")
.notNull()
.references(() => environment.id, { onDelete: "cascade" }),
channelId: uuid("channel_id")
.notNull()
.references(() => releaseChannel.id, { onDelete: "cascade" }),
+ deploymentId: uuid("deployment_id")
+ .notNull()
+ .references(() => deployment.id, { onDelete: "cascade" }),
},
(t) => ({
uniq: uniqueIndex().on(t.environmentId, t.channelId),
- deploymentUniq: uniqueIndex().on(
- t.environmentId,
- sql`(SELECT ${deployment.id} FROM ${releaseChannel} WHERE id = ${t.channelId})`,
- ),
+ deploymentUniq: uniqueIndex().on(t.environmentId, t.deploymentId),
}),
);This adjustment aligns the unique index definition with standard practices and ensures compatibility with the database.
📝 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.
| export const environmentReleaseChannel = pgTable( | |
| "environment_release_channel", | |
| { | |
| id: uuid("id").primaryKey().defaultRandom(), | |
| environmentId: uuid("environment_id") | |
| .notNull() | |
| .references(() => environment.id, { onDelete: "cascade" }), | |
| channelId: uuid("channel_id") | |
| .notNull() | |
| .references(() => releaseChannel.id, { onDelete: "cascade" }), | |
| }, | |
| (t) => ({ | |
| uniq: uniqueIndex().on(t.environmentId, t.channelId), | |
| deploymentUniq: uniqueIndex().on( | |
| t.environmentId, | |
| sql`(SELECT ${deployment.id} FROM ${releaseChannel} WHERE id = ${t.channelId})`, | |
| ), | |
| }), | |
| ); | |
| export const environmentReleaseChannel = pgTable( | |
| "environment_release_channel", | |
| { | |
| id: uuid("id").primaryKey().defaultRandom(), | |
| environmentId: uuid("environment_id") | |
| .notNull() | |
| .references(() => environment.id, { onDelete: "cascade" }), | |
| channelId: uuid("channel_id") | |
| .notNull() | |
| .references(() => releaseChannel.id, { onDelete: "cascade" }), | |
| deploymentId: uuid("deployment_id") | |
| .notNull() | |
| .references(() => deployment.id, { onDelete: "cascade" }), | |
| }, | |
| (t) => ({ | |
| uniq: uniqueIndex().on(t.environmentId, t.channelId), | |
| deploymentUniq: uniqueIndex().on(t.environmentId, t.deploymentId), | |
| }), | |
| ); |
| export const environmentPolicyReleaseChannel = pgTable( | ||
| "environment_policy_release_channel", | ||
| { | ||
| id: uuid("id").primaryKey().defaultRandom(), | ||
| policyId: uuid("policy_id") | ||
| .notNull() | ||
| .references(() => environmentPolicy.id, { onDelete: "cascade" }), | ||
| channelId: uuid("channel_id") | ||
| .notNull() | ||
| .references(() => releaseChannel.id, { onDelete: "cascade" }), | ||
| }, | ||
| (t) => ({ | ||
| uniq: uniqueIndex().on(t.policyId, t.channelId), | ||
| deploymentUniq: uniqueIndex().on( | ||
| t.policyId, | ||
| sql`(SELECT ${deployment.id} FROM ${releaseChannel} WHERE id = ${t.channelId})`, | ||
| ), | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Potential issue with unique index using a subquery
Defining deploymentUniq with a subquery in the uniqueIndex() may not be supported by the database and could lead to migration or runtime errors. Unique indexes typically operate directly on columns of the table.
Consider adjusting the schema to avoid using a subquery in the unique index. One approach is to add a deploymentId field to the table and use it in the unique index. Here's how you might modify the code:
export const environmentPolicyReleaseChannel = pgTable(
"environment_policy_release_channel",
{
id: uuid("id").primaryKey().defaultRandom(),
policyId: uuid("policy_id")
.notNull()
.references(() => environmentPolicy.id, { onDelete: "cascade" }),
channelId: uuid("channel_id")
.notNull()
.references(() => releaseChannel.id, { onDelete: "cascade" }),
+ deploymentId: uuid("deployment_id")
+ .notNull()
+ .references(() => deployment.id, { onDelete: "cascade" }),
},
(t) => ({
uniq: uniqueIndex().on(t.policyId, t.channelId),
- deploymentUniq: uniqueIndex().on(
- t.policyId,
- sql`(SELECT ${deployment.id} FROM ${releaseChannel} WHERE id = ${t.channelId})`,
- ),
+ deploymentUniq: uniqueIndex().on(t.policyId, t.deploymentId),
}),
);This change adds a deploymentId field to the table schema and modifies the unique index to use this field directly.
📝 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.
| export const environmentPolicyReleaseChannel = pgTable( | |
| "environment_policy_release_channel", | |
| { | |
| id: uuid("id").primaryKey().defaultRandom(), | |
| policyId: uuid("policy_id") | |
| .notNull() | |
| .references(() => environmentPolicy.id, { onDelete: "cascade" }), | |
| channelId: uuid("channel_id") | |
| .notNull() | |
| .references(() => releaseChannel.id, { onDelete: "cascade" }), | |
| }, | |
| (t) => ({ | |
| uniq: uniqueIndex().on(t.policyId, t.channelId), | |
| deploymentUniq: uniqueIndex().on( | |
| t.policyId, | |
| sql`(SELECT ${deployment.id} FROM ${releaseChannel} WHERE id = ${t.channelId})`, | |
| ), | |
| }), | |
| ); | |
| export const environmentPolicyReleaseChannel = pgTable( | |
| "environment_policy_release_channel", | |
| { | |
| id: uuid("id").primaryKey().defaultRandom(), | |
| policyId: uuid("policy_id") | |
| .notNull() | |
| .references(() => environmentPolicy.id, { onDelete: "cascade" }), | |
| channelId: uuid("channel_id") | |
| .notNull() | |
| .references(() => releaseChannel.id, { onDelete: "cascade" }), | |
| deploymentId: uuid("deployment_id") | |
| .notNull() | |
| .references(() => deployment.id, { onDelete: "cascade" }), | |
| }, | |
| (t) => ({ | |
| uniq: uniqueIndex().on(t.policyId, t.channelId), | |
| deploymentUniq: uniqueIndex().on(t.policyId, t.deploymentId), | |
| }), | |
| ); |
| @@ -90,11 +93,16 @@ export const environmentPolicy = pgTable("environment_policy", { | |||
| concurrencyType: concurrencyType("concurrency_type").notNull().default("all"), | |||
| concurrencyLimit: integer("concurrency_limit").notNull().default(1), | |||
|
|
|||
| duration: bigint("duration", { mode: "number" }).notNull().default(0), | |||
| // Duration in milliseconds over which to gradually roll out releases to this | |||
There was a problem hiding this comment.
wait are we doing seconds or milliseconds
There was a problem hiding this comment.
we can do seconds after we migrate this, low p
Summary by CodeRabbit
New Features
releaseChanneltable to manage release information.rolloutDuration,ephemeralDuration, andcreatedAtfields to enhance environment policy tracking.environment_policy_release_channelandenvironment_release_channelto support deployment channel relationships.Bug Fixes
releaseFilterfield, streamlining the environment policy schema.