-
Notifications
You must be signed in to change notification settings - Fork 11
init #462
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
init #462
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request updates the database queries and schema references to transition from using the Changes
Sequence Diagram(s)sequenceDiagram
participant Worker as EvaluateReleaseWorker
participant DB as Database (tx.query.versionRelease)
Worker->>DB: Invoke createJobForRelease(chosenReleaseId)
DB-->>Worker: Return versionRelease record
sequenceDiagram
participant Dispatch as GitHubDispatcher
participant DB as Database (Join Query)
Dispatch->>DB: Execute query joining releaseJob with versionRelease
DB-->>Dispatch: Return joined release job data
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
✨ 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/db/src/schema/release.ts (1)
57-63:variableReleaseintroduction is valid, though consider a creation timestamp default.
Currently,createdAtis optional. Using a.defaultNow()might enhance consistency with other tables.createdAt: timestamp("created_at", { withTimezone: true }) - + .defaultNow(),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/event-worker/src/workers/evaluate-release-target.ts(1 hunks)apps/event-worker/src/workers/job-dispatch/github.ts(1 hunks)packages/db/src/schema/release.ts(4 hunks)packages/rule-engine/src/repositories/db-release-repository.ts(1 hunks)packages/rule-engine/src/repositories/get-releases.ts(3 hunks)packages/rule-engine/src/repositories/types.ts(1 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/workers/evaluate-release-target.tspackages/rule-engine/src/repositories/db-release-repository.tspackages/rule-engine/src/repositories/types.tsapps/event-worker/src/workers/job-dispatch/github.tspackages/rule-engine/src/repositories/get-releases.tspackages/db/src/schema/release.ts
🧬 Code Definitions (3)
apps/event-worker/src/workers/evaluate-release-target.ts (1)
packages/db/src/schema/release.ts (1)
release(79-93)
packages/rule-engine/src/repositories/get-releases.ts (3)
packages/db/src/schema/job.ts (1)
JobStatus(136-136)packages/db/src/schema/release.ts (1)
releaseTarget(18-40)packages/db/src/schema/policy.ts (1)
policy(23-39)
packages/db/src/schema/release.ts (2)
packages/db/src/schema/deployment-version.ts (1)
deploymentVersion(109-134)packages/db/src/schema/job.ts (1)
job(74-106)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
🔇 Additional comments (25)
packages/rule-engine/src/repositories/types.ts (1)
9-9: Type definition updated to use the new schema entity.The
Releasetype definition has been updated to infer its structure fromschema.versionReleaseinstead ofschema.release, aligning with the broader schema restructuring across the codebase.apps/event-worker/src/workers/evaluate-release-target.ts (1)
22-24: Query updated to use versionRelease entity.The database query has been correctly updated to reference the new
versionReleaseentity instead of the previousreleaseentity, maintaining consistency with the schema changes.packages/rule-engine/src/repositories/db-release-repository.ts (2)
136-140: Insert operation updated to use versionRelease entity.The database insert operation has been correctly updated to target the new
schema.versionReleaseentity instead of the previousschema.releaseentity, aligning with the schema changes.
141-149: Variable storage entity updated to variableReleaseValue.The database insert operation for variables has been correctly updated to use the new
schema.variableReleaseValueentity instead of the previousschema.releaseVariableentity, maintaining consistency with the schema restructuring.apps/event-worker/src/workers/job-dispatch/github.ts (2)
77-80: Join operation updated to use versionRelease entity.The inner join in the database query has been correctly updated to join with the
SCHEMA.versionReleasetable instead of the previousSCHEMA.releasetable, aligning with the schema changes.
81-84: Join condition updated to reference versionRelease.The join condition between the
releaseTargettable and the release entity has been correctly updated to referenceSCHEMA.versionRelease.releaseTargetIdinstead of the previousSCHEMA.release.releaseTargetId, maintaining schema consistency.packages/rule-engine/src/repositories/get-releases.ts (11)
55-56: No immediate concerns with new references to thereleasetable.
The updated.from(schema.release)and.innerJoin(schema.job, eq(schema.release.jobId, schema.job.id))appear consistent with the newly introducedjobIdforeign key in the release table.
65-65: Ordering byversionRelease.createdAtlooks correct.
Ensures newer records appear first.
87-87: Switched to queryingversionReleaseis accurate.
Matches the updated schema for release tracking.
89-89: Filtering byreleaseTargetIdis valid.
Properly constrains the query to the correct release target relationship.
95-95: Ensuring only releases after the deployed release is logical.
gte(schema.versionRelease.createdAt, latestDeployedRelease.createdAt)correctly narrows the time window.
98-101: Bounded check against the desired release creation time is coherent.
Helps enforce an upper limit on retrieved releases.
108-108: Descending ordering byversionRelease.createdAtremains consistent.
No further issues here.
128-130: Returning the latest version release if no policy is specified looks good.
The usage of.findFirstwith descending order ensures you get the most recent entry.
136-136: QueryingversionReleaseagain is aligned with the updated schema.
No problems identified.
138-138: Filtering byreleaseTargetIdis again consistent.
Matches the same schema logic above.
145-145: Final descending sort step for.findFirstis coherent.
Keeps the latest release at the top of the result.packages/db/src/schema/release.ts (8)
42-55:versionReleasetable definition appears consistent.
ThereleaseTargetIdandversionIdreferences match the new schema design, and includingcreatedAtis standard practice.
65-77:variableReleaseValuewith unique(variableReleaseId, key)looks correct.
Ensures that each variable can only appear once per variable release.
79-93: Newreleasetable referencesversionReleaseandvariableReleaseappropriately.
The relationships tojoband timestamps are well-structured.
116-118: Extended relations onreleaseTargetreferencingversionReleasesandvariableReleasesare coherent.
Keeps the design consistent with the new table split.
121-134:versionReleaseRelationsaccurately captures links todeploymentVersion,releaseTarget, and the associatedrelease.
No issues found.
136-145:variableReleaseRelationslinking toreleaseTargetandreleaseis correct.
Ensures user can trace from avariableReleaseto all dependent relationships.
148-156:variableReleaseValuerelations are straightforward and properly referencevariableRelease.
No concerns.
158-171:releaseRelationsconnecting toversionRelease,variableRelease, andjobcompletes the schema remodeling.
This neatly consolidates the final link in the new structure.
| .where( | ||
| and( | ||
| sql`${schema.releaseJob.releaseId} = "releaseTarget_releases"."id"`, | ||
| sql`${schema.release.releaseId} = "releaseTarget_releases"."id"`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential mismatch referencing a non-existent releaseId column.
The new release table does not declare a releaseId field. Consider comparing id or versionReleaseId instead to avoid runtime errors.
- sql`${schema.release.releaseId} = "releaseTarget_releases"."id"`,
+ sql`${schema.release.id} = "releaseTarget_releases"."id"`,📝 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.
| sql`${schema.release.releaseId} = "releaseTarget_releases"."id"`, | |
| sql`${schema.release.id} = "releaseTarget_releases"."id"`, |
packages/db/src/schema/release.ts
Outdated
| releaseTargetId: uuid("release_target_id") | ||
| .notNull() | ||
| .references(() => releaseTarget.id, { onDelete: "cascade" }), | ||
| createdAt: timestamp("created_at", { withTimezone: true }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| createdAt: timestamp("created_at", { withTimezone: true }), | |
| createdAt: timestamp("created_at", { withTimezone: true }).nonNull(), |
Summary by CodeRabbit