Skip to content

fix: get latest release state by joining on release table#631

Merged
adityachoudhari26 merged 1 commit intomainfrom
redeploy-noop-bug
Jul 29, 2025
Merged

fix: get latest release state by joining on release table#631
adityachoudhari26 merged 1 commit intomainfrom
redeploy-noop-bug

Conversation

@adityachoudhari26
Copy link
Copy Markdown
Member

@adityachoudhari26 adityachoudhari26 commented Jul 29, 2025

Summary by CodeRabbit

  • Refactor
    • Improved the process for determining and creating new releases, resulting in clearer and more reliable release evaluation.
    • Enhanced logic for comparing releases, ensuring more accurate detection of changes.
    • Streamlined release management for better performance and maintainability.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Jul 29, 2025

Walkthrough

The code refactors the release evaluation workflow in evaluate-release-target.ts by introducing helper functions for fetching and inserting releases, comparing release states, and improving the structure of related entity queries. The main worker logic now uses these helpers to decide whether to insert a new release or reuse the existing one based on version and variable changes.

Changes

Cohort / File(s) Change Summary
Release Evaluation Refactor
apps/event-worker/src/workers/evaluate-release-target.ts
Added helper functions: getReleaseTarget, getCurrentRelease, getHasAnythingChanged, and insertNewRelease. Refactored main worker logic to use these helpers, improving clarity and structure of release evaluation and insertion. Enhanced entity fetching with joined queries.

Sequence Diagram(s)

sequenceDiagram
    participant Worker
    participant DB
    Worker->>DB: getReleaseTarget(resourceId, environmentId, deploymentId)
    DB-->>Worker: releaseTarget
    Worker->>DB: Lock releaseTarget
    Worker->>DB: Upsert versionRelease & variableRelease (concurrently)
    Worker->>DB: getCurrentRelease(releaseTargetId)
    DB-->>Worker: currentRelease (with joined entities)
    alt No currentRelease
        Worker->>DB: insertNewRelease(versionReleaseId, variableReleaseId)
        DB-->>Worker: newRelease
        Worker-->>Worker: return newRelease
    else Has changes
        Worker->>DB: insertNewRelease(versionReleaseId, variableReleaseId)
        DB-->>Worker: newRelease
        Worker-->>Worker: return newRelease
    else No changes
        Worker-->>Worker: return currentRelease
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~15 minutes

Possibly related PRs

Suggested reviewers

  • jsbroks

Poem

In the warren of code, a new path is found,
With helpers and queries all neatly wound.
Releases compared, no more repeats,
The logic now hops with elegant feats.
🐇✨
Refactored and tidy, this code’s a delight—
A rabbit’s approval, crisp and bright!

Note

⚡️ Unit Test Generation is now available in beta!

Learn more here, or try it out under "Finishing Touches" below.


📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f90670f and 22058d8.

📒 Files selected for processing (1)
  • apps/event-worker/src/workers/evaluate-release-target.ts (5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}

📄 CodeRabbit Inference Engine (CLAUDE.md)

**/*.{ts,tsx}: Use TypeScript with explicit types (prefer interfaces for public APIs)
Import styles: Use named imports, group imports by source (std lib > external > internal)
Consistent type imports: import type { Type } from "module"
Prefer async/await over raw promises
Handle errors explicitly (use try/catch and typed error responses)

Files:

  • apps/event-worker/src/workers/evaluate-release-target.ts

⚙️ CodeRabbit Configuration File

**/*.{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.

Files:

  • apps/event-worker/src/workers/evaluate-release-target.ts
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}

📄 CodeRabbit Inference Engine (CLAUDE.md)

Formatting: Prettier is used with @ctrlplane/prettier-config

Files:

  • apps/event-worker/src/workers/evaluate-release-target.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#627
File: apps/event-worker/src/workers/job-update/trigger-dependendent-targets.ts:0-0
Timestamp: 2025-07-23T22:42:20.933Z
Learning: In apps/event-worker/src/workers/job-update/trigger-dependendent-targets.ts, when implementing error handling for the triggerDependentTargets function, only a try-catch wrapper is needed around the entire function body. Additional null checks after takeFirst() calls are unnecessary since takeFirst throws an error if no record is found.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#237
File: packages/api/src/router/job.ts:362-365
Timestamp: 2024-11-27T23:18:42.055Z
Learning: In the file `packages/api/src/router/job.ts`, the function `releaseMatchesCondition` returns undefined if the `filter` parameter is null. This behavior ensures that when constructing the query with `and(...)`, the condition is omitted, allowing the query to function correctly even if there is no release channel associated with the environment.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#181
File: packages/api/src/router/deployment.ts:116-131
Timestamp: 2024-10-29T02:04:50.312Z
Learning: In `packages/api/src/router/deployment.ts`, the `list.byDeploymentId` procedure requires multiple database queries due to limitations of the `releaseMatchesCondition` function.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#237
File: packages/api/src/router/job.ts:382-390
Timestamp: 2024-11-27T23:18:59.456Z
Learning: In the codebase, releases are not scoped to a specific resource.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#188
File: apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/Usage.tsx:42-44
Timestamp: 2024-11-01T02:35:07.352Z
Learning: In `apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/Usage.tsx`, within the `Usage` component, the arrays used for filtering inherited environments are expected to remain small. Therefore, performance optimizations for this filtering logic are not necessary.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#601
File: e2e/tests/api/policies/retry-policy.spec.ts:23-24
Timestamp: 2025-06-24T23:52:50.732Z
Learning: The user adityachoudhari26 prefers not to add null safety checks or defensive programming in test code, particularly in e2e tests, as they prioritize simplicity and focus on the main functionality being tested rather than comprehensive error handling within the test itself.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#601
File: e2e/tests/api/policies/retry-policy.spec.ts:117-130
Timestamp: 2025-06-24T23:53:25.398Z
Learning: User adityachoudhari26 prefers to keep non-null assertions in e2e test code without extensive null safety checks, reasoning that test failures serve the same purpose of catching issues and the extra validation doesn't add much value in test contexts.
apps/event-worker/src/workers/evaluate-release-target.ts (8)

Learnt from: adityachoudhari26
PR: #627
File: apps/event-worker/src/workers/job-update/trigger-dependendent-targets.ts:0-0
Timestamp: 2025-07-23T22:42:20.933Z
Learning: In apps/event-worker/src/workers/job-update/trigger-dependendent-targets.ts, when implementing error handling for the triggerDependentTargets function, only a try-catch wrapper is needed around the entire function body. Additional null checks after takeFirst() calls are unnecessary since takeFirst throws an error if no record is found.

Learnt from: adityachoudhari26
PR: #601
File: packages/job-dispatch/src/job-update.ts:264-270
Timestamp: 2025-06-24T23:56:54.799Z
Learning: In this codebase, the Tx type is just an alias for the database client type (Omit<typeof db, "$client">) and does not necessarily indicate an active transaction context. Functions like createReleaseJob need to be called within a transaction, which is why they are wrapped with db.transaction() even when the parameter is typed as Tx. Drizzle supports nested transactions via breakpoints, so additional transaction wrappers are safe even if already within a transaction.

Learnt from: adityachoudhari26
PR: #181
File: packages/api/src/router/deployment.ts:116-131
Timestamp: 2024-10-29T02:04:50.312Z
Learning: In packages/api/src/router/deployment.ts, the list.byDeploymentId procedure requires multiple database queries due to limitations of the releaseMatchesCondition function.

Learnt from: adityachoudhari26
PR: #237
File: packages/api/src/router/job.ts:362-365
Timestamp: 2024-11-27T23:18:42.055Z
Learning: In the file packages/api/src/router/job.ts, the function releaseMatchesCondition returns undefined if the filter parameter is null. This behavior ensures that when constructing the query with and(...), the condition is omitted, allowing the query to function correctly even if there is no release channel associated with the environment.

Learnt from: adityachoudhari26
PR: #604
File: packages/rule-engine/src/manager/version-manager.ts:124-139
Timestamp: 2025-06-30T21:19:43.866Z
Learning: In packages/rule-engine/src/manager/version-manager.ts, the findDesiredVersion method should use takeFirst (not takeFirstOrNull) because if a desiredVersionId is set but the query fails to find exactly one matching version, it indicates a data integrity or configuration issue that should fail loudly rather than be handled silently.

Learnt from: adityachoudhari26
PR: #579
File: packages/rule-engine/src/rules/concurrency-rule.ts:8-11
Timestamp: 2025-06-01T19:10:47.122Z
Learning: In packages/rule-engine/src/rules/concurrency-rule.ts, the ConcurrencyRule should remain simple without additional validation since database and Zod schemas already handle concurrency validation. The user prefers this rule to be "dumb" and just perform the comparison check rather than duplicating validation logic.

Learnt from: adityachoudhari26
PR: #395
File: packages/api/src/router/environment-page/resources/router.ts:40-45
Timestamp: 2025-03-24T18:46:38.894Z
Learning: The takeFirst utility function in the codebase (from @ctrlplane/db) throws an Error with message "Found non unique or inexistent value" if the result array doesn't contain exactly one element, making additional null/undefined checks unnecessary after its use.

Learnt from: adityachoudhari26
PR: #579
File: packages/db/src/schema/rules/concurrency.ts:0-0
Timestamp: 2025-06-01T19:10:11.535Z
Learning: In the ctrlplane codebase, when defining database schemas with Drizzle ORM, it's an intentional pattern to spread base fields (like basePolicyRuleFields) and then redefine specific fields to add additional constraints (like unique constraints or foreign key references). The TypeScript field overwriting behavior is deliberately used to override base field definitions with more specific requirements.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: Typecheck
  • GitHub Check: Lint
  • GitHub Check: build (linux/amd64)
🔇 Additional comments (7)
apps/event-worker/src/workers/evaluate-release-target.ts (7)

5-5: LGTM! Import addition looks correct.

The addition of takeFirstOrNull import is appropriate for the new getCurrentRelease function implementation.


25-52: Helper function implementation is solid.

The getReleaseTarget function properly encapsulates the release target fetching logic with appropriate error handling and related entity joins. The error message provides clear debugging information with all relevant identifiers.


168-177: LGTM! insertNewRelease helper is clean and correct.

The function properly inserts a new release record and uses takeFirst appropriately to ensure exactly one record is returned.


197-197: Good refactoring to use the helper function.

Replacing the inline query with the getReleaseTarget helper improves code readability and maintainability.


208-220: Logic depends on the getCurrentRelease implementation.

The refactored transaction logic using the helper functions is cleaner and more readable. However, the correctness depends on the getCurrentRelease function working properly and the getHasAnythingChanged logic being fixed.

Once the getHasAnythingChanged logic is fixed and the getCurrentRelease joins are verified, this refactored logic should work correctly. The overall flow is:

  1. Get current release (or null if none exists)
  2. If no current release, insert new one
  3. If current release exists, check if anything changed
  4. If nothing changed, return current release; otherwise insert new release

This logic is sound and improves upon the previous implementation.


154-166: Logic error in getHasAnythingChanged function.

The function has inverted logic - it should return true when something has changed, but the current implementation returns true when nothing has changed.

Apply this diff to fix the logic:

-const getHasAnythingChanged = (
-  currentRelease: {
-    currentVersionRelease: { id: string };
-    currentVariableRelease: { id: string };
-  },
-  newRelease: { versionReleaseId: string; variableReleaseId: string },
-) => {
-  const isVersionUnchanged =
-    currentRelease.currentVersionRelease.id === newRelease.versionReleaseId;
-  const areVariablesUnchanged =
-    currentRelease.currentVariableRelease.id === newRelease.variableReleaseId;
-  return !isVersionUnchanged || !areVariablesUnchanged;
-};
+const getHasAnythingChanged = (
+  currentRelease: {
+    currentVersionRelease: { id: string };
+    currentVariableRelease: { id: string };
+  },
+  newRelease: { versionReleaseId: string; variableReleaseId: string },
+) => {
+  const isVersionChanged =
+    currentRelease.currentVersionRelease.id !== newRelease.versionReleaseId;
+  const areVariablesChanged =
+    currentRelease.currentVariableRelease.id !== newRelease.variableReleaseId;
+  return isVersionChanged || areVariablesChanged;
+};
⛔ Skipped due to learnings
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#604
File: packages/rule-engine/src/manager/version-manager.ts:124-139
Timestamp: 2025-06-30T21:19:43.866Z
Learning: In packages/rule-engine/src/manager/version-manager.ts, the findDesiredVersion method should use takeFirst (not takeFirstOrNull) because if a desiredVersionId is set but the query fails to find exactly one matching version, it indicates a data integrity or configuration issue that should fail loudly rather than be handled silently.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#237
File: packages/api/src/router/job.ts:362-365
Timestamp: 2024-11-27T23:18:42.055Z
Learning: In the file `packages/api/src/router/job.ts`, the function `releaseMatchesCondition` returns undefined if the `filter` parameter is null. This behavior ensures that when constructing the query with `and(...)`, the condition is omitted, allowing the query to function correctly even if there is no release channel associated with the environment.

128-152: getCurrentRelease join conditions and result aliasing are correct

All joins line up with the schema definitions:

  • release.versionReleaseId → version_release.id
  • release.variableReleaseId → variable_set_release.id

The returned object keys (release, version_release, variable_set_release) match the table names, and your destructuring into currentVersionRelease and currentVariableRelease is accurate.

Optional improvement (only if you need to guard against mismatched targets):
• Add a filter on the variable‐set side to ensure it’s scoped to the same releaseTargetId:

.where(eq(schema.variableSetRelease.releaseTargetId, releaseTargetId))
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch redeploy-noop-bug

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need 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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai generate unit tests to generate unit tests for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@adityachoudhari26 adityachoudhari26 merged commit d919f3b into main Jul 29, 2025
6 checks passed
@adityachoudhari26 adityachoudhari26 deleted the redeploy-noop-bug branch July 29, 2025 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant