clean up releases db #439
clean up releases db #439
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis update modifies the database schema across multiple modules. The Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (4)
✨ Finishing Touches
🪧 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: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/db/src/schema/deployment.ts(0 hunks)packages/db/src/schema/release.ts(4 hunks)packages/db/src/schema/resource.ts(2 hunks)
💤 Files with no reviewable changes (1)
- packages/db/src/schema/deployment.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.
packages/db/src/schema/resource.tspackages/db/src/schema/release.ts
🧬 Code Definitions (2)
packages/db/src/schema/resource.ts (1)
packages/db/src/schema/release.ts (1)
releaseTarget(18-40)
packages/db/src/schema/release.ts (3)
packages/db/src/schema/resource.ts (1)
resource(59-87)packages/db/src/schema/environment.ts (1)
environment(57-82)packages/db/src/schema/deployment.ts (1)
deployment(66-90)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (6)
packages/db/src/schema/release.ts (4)
1-1: No issues with the new import.This addition aligns with the rest of the import statements, and the usage of
relationsandsqlis appropriate.
18-40: Verify forward reference and type usage fordesiredReleaseId.The table definition appears consistent with your schema approach, but please ensure there's no runtime or type issue from referencing
releasein line 34 via(): any => release.id.
45-47: Confirm application logic forreleaseTargetIdreferences.Declaring
releaseTargetIdasNOT NULLis fine, but make sure that upstream creation properly includes a validreleaseTargetIdto avoid foreign key constraint errors.
89-91: Good relationship mapping.This direct relationship from
releasetoreleaseTargetlooks correct. It should help streamline queries involving releases and their respective targets.packages/db/src/schema/resource.ts (2)
55-55: Import for newreleaseTargetreference.The import statement is correct and ensures that the
release.tsdefinitions are accessible for the updated resource relations.
102-102:releaseTargetsrelation properly replaces the legacy concept.Transitioning to
releaseTargetsaligns with the restructured schema. This provides a clear, direct relationship to the newrelease_targettable without any apparent issues.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/db/src/schema/release.ts (2)
18-40: Consider eliminating the: anycast fordesiredReleaseId.
The statementreferences((): any => release.id, ...)indicates a forward reference workaround. If feasible, reordering definitions or extracting shared types might avoid casting toany.
118-140: Specify references for thereleasesrelationship for clarity.
While Drizzle can infer the linking columns, explicitly defining them (e.g.,fields: [release.releaseTargetId], references: [releaseTarget.id]) may increase readability.packages/release-manager/src/repositories/release-repository.ts (1)
30-54: Optimize retrieval of the most recent release.
The query currently fetches all associated releases, only to return.at(0). Consider limiting the sub-query to mitigate unnecessary data retrieval, for example:with: { releases: { + limit: 1, with: { version: true, variables: true, }, }, },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/event-worker/src/releases/variable-change/index.ts(3 hunks)packages/db/src/schema/release.ts(4 hunks)packages/release-manager/src/repositories/release-repository.ts(5 hunks)packages/release-manager/src/types.ts(1 hunks)packages/rule-engine/src/db/create-ctx.ts(2 hunks)packages/rule-engine/src/utils/get-releases.ts(2 hunks)
🧰 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.
apps/event-worker/src/releases/variable-change/index.tspackages/rule-engine/src/db/create-ctx.tspackages/release-manager/src/types.tspackages/rule-engine/src/utils/get-releases.tspackages/db/src/schema/release.tspackages/release-manager/src/repositories/release-repository.ts
🧬 Code Definitions (4)
packages/rule-engine/src/db/create-ctx.ts (1)
packages/db/src/schema/release.ts (1)
releaseTarget(18-40)
packages/rule-engine/src/utils/get-releases.ts (1)
packages/db/src/schema/release.ts (1)
releaseTarget(18-40)
packages/db/src/schema/release.ts (3)
packages/db/src/schema/resource.ts (1)
resource(59-87)packages/db/src/schema/environment.ts (1)
environment(57-82)packages/db/src/schema/deployment.ts (1)
deployment(66-90)
packages/release-manager/src/repositories/release-repository.ts (2)
packages/release-manager/src/types.ts (2)
ReleaseIdentifier(3-7)Release(9-13)packages/db/src/schema/release.ts (2)
releaseTarget(18-40)release(42-55)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (15)
packages/release-manager/src/types.ts (1)
9-13: LGTM: Addition of releaseTargetId to Release type.This change adds a new required field
releaseTargetIdto theReleasetype, which aligns with the database schema updates that introduce the newreleaseTargettable.packages/rule-engine/src/db/create-ctx.ts (1)
4-4: Import statement updated to use releaseTarget instead of resourceRelease.This update correctly reflects the schema changes where resourceRelease has been removed in favor of releaseTarget.
apps/event-worker/src/releases/variable-change/index.ts (3)
19-30: Query and result mapping updated from resourceRelease to releaseTarget.The function has been properly updated to query from the new releaseTarget table and join it with the resource table. The result mapping has also been updated to extract data from release_target instead of resource_release.
41-43: LGTM: Updated deploymentId referenceThe code now correctly references releaseTarget.deploymentId in the where clause.
57-60: LGTM: Updated resourceId referenceThe code now correctly references releaseTarget.resourceId in the where clause.
packages/rule-engine/src/utils/get-releases.ts (4)
54-61: LGTM: Query updated from resourceRelease to releaseTarget.The database query has been correctly updated to use the new releaseTarget table, with proper where clauses and relation loading.
63-66: LGTM: Variable reference updated for date bounds check.References to resourceRelease have been properly updated to releaseTarget in the date bounds validation.
68-73: LGTM: Log message updated to reference releaseTarget.The warning log message has been updated to correctly reference releaseTarget instead of resourceRelease.
88-93: LGTM: Condition updated to use releaseTarget.The condition for filtering releases by creation date now correctly references releaseTarget instead of resourceRelease.
packages/db/src/schema/release.ts (3)
1-1: No remarks needed.
45-47: Foreign key reference is aligned with the new table.
ThereleaseTargetIdfield provides a clear one-to-one link toreleaseTarget.
89-91: One-to-one relationship is properly defined.
This matches the newly introducedreleaseTargetIdcolumn on thereleasetable.packages/release-manager/src/repositories/release-repository.ts (3)
73-84: Clean helper method to fetch the release target.
No issues found.
94-98: Proper null check for the target.
EnsuringreleaseTargetexists before creating a release prevents orphan releases.
143-156: Upsert logic appears correct.
Conflict resolution is appropriately handled via the unique index on(environmentId, deploymentId, resourceId).
| return tx.query.releaseTarget.findFirst({ | ||
| where: and( | ||
| eq(resourceRelease.id, repo.resourceId), | ||
| eq(resourceRelease.environmentId, repo.environmentId), | ||
| eq(resourceRelease.deploymentId, repo.deploymentId), | ||
| eq(releaseTarget.id, repo.resourceId), | ||
| eq(releaseTarget.environmentId, repo.environmentId), | ||
| eq(releaseTarget.deploymentId, repo.deploymentId), | ||
| ), |
There was a problem hiding this comment.
Database query incorrectly mapping resourceId to releaseTarget.id
The query is comparing releaseTarget.id with repo.resourceId, but based on the releaseTarget schema from the relevant code snippets, it should be comparing releaseTarget.resourceId with repo.resourceId.
return tx.query.releaseTarget.findFirst({
where: and(
- eq(releaseTarget.id, repo.resourceId),
+ eq(releaseTarget.resourceId, repo.resourceId),
eq(releaseTarget.environmentId, repo.environmentId),
eq(releaseTarget.deploymentId, repo.deploymentId),
),📝 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.
| return tx.query.releaseTarget.findFirst({ | |
| where: and( | |
| eq(resourceRelease.id, repo.resourceId), | |
| eq(resourceRelease.environmentId, repo.environmentId), | |
| eq(resourceRelease.deploymentId, repo.deploymentId), | |
| eq(releaseTarget.id, repo.resourceId), | |
| eq(releaseTarget.environmentId, repo.environmentId), | |
| eq(releaseTarget.deploymentId, repo.deploymentId), | |
| ), | |
| return tx.query.releaseTarget.findFirst({ | |
| where: and( | |
| eq(releaseTarget.resourceId, repo.resourceId), | |
| eq(releaseTarget.environmentId, repo.environmentId), | |
| eq(releaseTarget.deploymentId, repo.deploymentId), | |
| ), |
Summary by CodeRabbit
New Features
releaseTargettable to manage release targets effectively.policy_deployment_version_selectortable for improved policy management.Refactor