Skip to content
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

fix: Convert completed to successful #287

Merged
merged 2 commits into from
Jan 21, 2025

Conversation

adityachoudhari26
Copy link
Contributor

@adityachoudhari26 adityachoudhari26 commented Jan 21, 2025

Summary by CodeRabbit

Release Notes

  • New Features

    • None
  • Improvements

    • Updated job status terminology from "Completed" to "Successful" across the platform
    • Enhanced job status tracking with new timestamp fields for job start and completion times
  • Bug Fixes

    • None
  • Chores

    • Refactored job status handling in multiple components and services
    • Updated database schema and API specifications to reflect new job status terminology

Copy link
Contributor

coderabbitai bot commented Jan 21, 2025

Walkthrough

This pull request introduces a comprehensive update to job status handling across the Ctrlplane application. The primary change involves replacing the JobStatus.Completed status with JobStatus.Successful throughout the codebase. This modification spans multiple files, including frontend components, API handlers, database schemas, and validation logic. The changes ensure consistent terminology and improve the precision of job status representation, affecting how job completions are tracked, displayed, and processed.

Changes

File Path Change Summary
apps/webservice/src/* Updated job status checks from Completed to Successful in multiple UI components
packages/db/drizzle/0060_stale_warlock.sql Renamed job status type value from 'completed' to 'successful'
packages/validators/src/jobs/index.ts Modified JobStatus enum and related mappings
packages/job-dispatch/src/* Updated job completion and status checking logic
apps/api/v1/openapi.ts Added startedAt and completedAt properties to Job schema

Sequence Diagram

sequenceDiagram
    participant UI as User Interface
    participant API as API Handler
    participant DB as Database
    participant Validator as Job Status Validator

    UI->>Validator: Request job status check
    Validator->>API: Validate job status
    API->>DB: Query jobs with new status
    DB-->>API: Return jobs
    API-->>UI: Render job status as 'Successful'
Loading

Possibly related PRs

  • fix: Show different statuses on graph #156: The changes in the main PR regarding the rendering of job statuses are related to the modifications in the JobHistoryChart.tsx, which also involves job status representation.
  • fix: Release dependencies refactor #165: The updates in the main PR about job status checks are connected to the changes in the CreateRelease.tsx, which also deals with job statuses.
  • fix: Optimize flow node query #190: The changes in the main PR regarding job status checks relate to the modifications in the FlowDiagram.tsx, which also involves job status handling.

Suggested reviewers

  • jsbroks

Poem

🐰 A Rabbit's Ode to Job Status Clarity

From 'Completed' to 'Successful' we leap,
Precision in status, no more to keep
Ambiguous terms that muddle the way,
Our code now sings with clarity today!
🎉 Hop, hop, hooray for clean status display! 🚀


📜 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 c6cfaff and 458d6fe.

📒 Files selected for processing (1)
  • apps/webservice/src/app/[workspaceSlug]/(app)/_components/resource-drawer/DeploymentContent.tsx (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/webservice/src/app/[workspaceSlug]/(app)/_components/resource-drawer/DeploymentContent.tsx
⏰ Context from checks skipped due to timeout of 90000ms (5)
  • GitHub Check: Typecheck
  • GitHub Check: Lint
  • GitHub Check: Format
  • GitHub Check: build (linux/amd64)
  • GitHub Check: build (linux/amd64)

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?

❤️ 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.
    • Generate unit testing code for this file.
    • 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 generate unit testing code for this file.
    • @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 generate unit testing code.
    • @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.

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. (Beta)
  • @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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (4)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowNode.tsx (1)

43-43: Consider enhancing the badge with status colors.

The successful/total ratio display is correct. Consider enhancing the UX by color-coding the badge based on the success ratio (e.g., green when all jobs are successful).

-            {successful?.length} / {environmentJobs?.length}
+            <span className={cn(
+              "text-muted-foreground",
+              successful?.length === environmentJobs?.length && "text-green-500"
+            )}>
+              {successful?.length} / {environmentJobs?.length}
+            </span>
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowPolicyNode.tsx (1)

Line range hint 54-54: Fix typo in success message.

There's a typo in the word "sucessfully" (missing a 'c').

-        completed sucessfully
+        completed successfully
packages/job-dispatch/src/queue/db.ts (1)

7-7: LGTM! Good transition to enum usage.

The change improves type safety by using JobStatus enum instead of string literals. Consider extracting the excluded statuses array to a constant if it's used elsewhere in the codebase.

+const EXCLUDED_JOB_STATUSES = [
+  JobStatus.Failure,
+  JobStatus.Cancelled,
+  JobStatus.Skipped,
+  JobStatus.Successful,
+  JobStatus.InvalidJobAgent,
+] as const;

 async next(agentId: string): Promise<Job[]> {
   const jobs = await this.db
     .select()
     .from(job)
     .where(
       and(
         eq(job.jobAgentId, agentId),
-        notInArray(job.status, [
-          JobStatus.Failure,
-          JobStatus.Cancelled,
-          JobStatus.Skipped,
-          JobStatus.Successful,
-          JobStatus.InvalidJobAgent,
-        ]),
+        notInArray(job.status, EXCLUDED_JOB_STATUSES),
         isNull(job.externalId),
       ),
     );

Also applies to: 28-32

packages/validators/src/jobs/index.ts (1)

8-8: LGTM! Consider updating documentation.

The status change is consistently applied across the enum, readable mapping, and exitedStatus array. Consider updating any API documentation or user guides that might reference the old "completed" status.

Run this script to find documentation files that might need updates:

#!/bin/bash
# Search for documentation files containing "completed" status
rg -i "completed" --type md --type mdx --type txt

Also applies to: 23-23, 39-39

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 8677f43 and c6cfaff.

📒 Files selected for processing (25)
  • apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resources/[resourceId]/ReleaseCell.tsx (1 hunks)
  • apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resources/[resourceId]/visualize/nodes/DeploymentNode.tsx (4 hunks)
  • apps/webservice/src/app/[workspaceSlug]/(app)/_components/DailyJobsChart.tsx (2 hunks)
  • apps/webservice/src/app/[workspaceSlug]/(app)/_components/JobTableStatusIcon.tsx (1 hunks)
  • apps/webservice/src/app/[workspaceSlug]/(app)/_components/job-condition/JobComparisonConditionRender.tsx (1 hunks)
  • apps/webservice/src/app/[workspaceSlug]/(app)/_components/job-condition/RunbookJobComparisonConditionRender.tsx (1 hunks)
  • apps/webservice/src/app/[workspaceSlug]/(app)/_components/resource-drawer/DeploymentContent.tsx (1 hunks)
  • apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/TableCells.tsx (1 hunks)
  • apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowNode.tsx (2 hunks)
  • apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowPolicyNode.tsx (2 hunks)
  • apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/status-color.tsx (1 hunks)
  • apps/webservice/src/app/api/github/webhook/workflow/handler.ts (1 hunks)
  • apps/webservice/src/app/api/v1/job-agents/[agentId]/jobs/running/route.ts (2 hunks)
  • apps/webservice/src/app/api/v1/openapi.ts (2 hunks)
  • openapi.v1.json (2 hunks)
  • packages/api/src/router/deployment.ts (1 hunks)
  • packages/db/drizzle/0060_stale_warlock.sql (1 hunks)
  • packages/db/drizzle/meta/_journal.json (1 hunks)
  • packages/db/src/schema/job.ts (1 hunks)
  • packages/job-dispatch/src/job-update.ts (1 hunks)
  • packages/job-dispatch/src/policies/release-dependency.ts (1 hunks)
  • packages/job-dispatch/src/policies/success-rate-criteria-passing.ts (1 hunks)
  • packages/job-dispatch/src/queue/db.ts (2 hunks)
  • packages/validators/src/jobs/conditions/status-condition.ts (1 hunks)
  • packages/validators/src/jobs/index.ts (3 hunks)
✅ Files skipped from review due to trivial changes (1)
  • packages/db/drizzle/0060_stale_warlock.sql
🧰 Additional context used
📓 Path-based instructions (22)
apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resources/[resourceId]/ReleaseCell.tsx (1)

Pattern **/*.{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/webservice/src/app/[workspaceSlug]/(app)/_components/JobTableStatusIcon.tsx (1)

Pattern **/*.{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/api/src/router/deployment.ts (1)

Pattern **/*.{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/validators/src/jobs/conditions/status-condition.ts (1)

Pattern **/*.{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/job-dispatch/src/policies/release-dependency.ts (1)

Pattern **/*.{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/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowNode.tsx (1)

Pattern **/*.{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/job-dispatch/src/job-update.ts (1)

Pattern **/*.{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/job.ts (1)

Pattern **/*.{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/webservice/src/app/[workspaceSlug]/(app)/_components/resource-drawer/DeploymentContent.tsx (1)

Pattern **/*.{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/webservice/src/app/api/v1/openapi.ts (1)

Pattern **/*.{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/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/status-color.tsx (1)

Pattern **/*.{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/webservice/src/app/[workspaceSlug]/(app)/(resources)/resources/[resourceId]/visualize/nodes/DeploymentNode.tsx (1)

Pattern **/*.{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/webservice/src/app/[workspaceSlug]/(app)/_components/job-condition/JobComparisonConditionRender.tsx (1)

Pattern **/*.{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/webservice/src/app/[workspaceSlug]/(app)/_components/DailyJobsChart.tsx (1)

Pattern **/*.{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/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/TableCells.tsx (1)

Pattern **/*.{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/webservice/src/app/[workspaceSlug]/(app)/_components/job-condition/RunbookJobComparisonConditionRender.tsx (1)

Pattern **/*.{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/validators/src/jobs/index.ts (1)

Pattern **/*.{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/webservice/src/app/api/v1/job-agents/[agentId]/jobs/running/route.ts (1)

Pattern **/*.{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/job-dispatch/src/queue/db.ts (1)

Pattern **/*.{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/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowPolicyNode.tsx (1)

Pattern **/*.{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/webservice/src/app/api/github/webhook/workflow/handler.ts (1)

Pattern **/*.{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/job-dispatch/src/policies/success-rate-criteria-passing.ts (1)

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

⏰ Context from checks skipped due to timeout of 90000ms (3)
  • GitHub Check: Typecheck
  • GitHub Check: Lint
  • GitHub Check: build (linux/amd64)
🔇 Additional comments (26)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowNode.tsx (1)

26-27: LGTM! Status filter updated correctly.

The change from completed to successful status is implemented correctly with proper null safety.

apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowPolicyNode.tsx (3)

47-48: LGTM! The status check is now more precise.

The change from checking completed to successful status provides a more accurate count of passing jobs.


59-60: LGTM! Variable name accurately reflects the status check.

The change from areAllCompleted to areAllSuccessful maintains semantic clarity while implementing the status change.


64-66: LGTM! UI text consistently reflects the status change.

The message update from "completed" to "successful" maintains consistency with the new status terminology.

apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/status-color.tsx (1)

5-5: LGTM! Color mapping maintained correctly.

The status color mapping has been updated from "completed" to "successful" while maintaining the appropriate green[400] color for success state.

apps/webservice/src/app/[workspaceSlug]/(app)/_components/JobTableStatusIcon.tsx (1)

17-20: LGTM! Status terminology update is consistent.

The change from JobStatus.Completed to JobStatus.Successful maintains the same visual feedback while improving status terminology precision.

apps/webservice/src/app/api/v1/job-agents/[agentId]/jobs/running/route.ts (1)

42-46: LGTM! Verify database queries still work as expected.

The status update from "completed" to "successful" in the query filter is consistent with the terminology change.

Let's verify that no jobs are stuck in an old "completed" status:

✅ Verification successful

Database migration properly handles the status rename

The migration file packages/db/drizzle/0060_stale_warlock.sql ensures all jobs with 'completed' status are renamed to 'successful'.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if any jobs still have the old "completed" status
rg -i "status.*completed" --type sql

Length of output: 395

apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resources/[resourceId]/ReleaseCell.tsx (1)

Line range hint 40-46: LGTM! Status update is consistent with enum usage.

The change from JobStatus.Completed to JobStatus.Successful is properly implemented using the JobStatus enum.

apps/webservice/src/app/api/github/webhook/workflow/handler.ts (1)

12-17: LGTM! Consistent mapping of GitHub workflow statuses.

The changes correctly map GitHub workflow statuses to our internal JobStatus enum:

  • GitHub's "success" conclusion → JobStatus.Successful
  • GitHub's "completed" status → JobStatus.Successful

Also applies to: 23-23

packages/job-dispatch/src/policies/success-rate-criteria-passing.ts (1)

41-41: LGTM! Consistent success criteria evaluation.

The changes correctly update the success criteria evaluation to use JobStatus.Successful while maintaining the original logic for policy evaluation.

Also applies to: 44-46

packages/job-dispatch/src/job-update.ts (1)

113-114: Verify impact on job completion handlers.

The change from JobStatus.Completed to JobStatus.Successful affects when onJobCompletion is triggered. Please verify that all downstream processes expecting job completion events are updated accordingly.

apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resources/[resourceId]/visualize/nodes/DeploymentNode.tsx (1)

42-43: LGTM! Consistent UI status representation.

The changes correctly update the deployment node visualization to reflect JobStatus.Successful:

  • Status check logic updated
  • CSS classes updated consistently

Also applies to: 55-55, 84-84, 96-96

apps/webservice/src/app/api/v1/openapi.ts (2)

121-121: LGTM! Status enum value updated consistently.

The change from "completed" to "successful" in the Job schema's status enum aligns with the PR objective.


141-142: Verify API consumers are prepared for the new timing fields.

The addition of startedAt and completedAt fields enhances job tracking capabilities. However, ensure that API consumers are notified of these additions.

Consider documenting the behavior of these fields:

  • When exactly is startedAt set?
  • What triggers the update of completedAt?
  • Are there any scenarios where one field might be set without the other?
packages/job-dispatch/src/policies/release-dependency.ts (1)

125-125: Verify deployment flows with the updated policy check.

The change from JobStatus.Completed to JobStatus.Successful in the dependency policy check is logically correct but could affect existing deployment flows.

Run this script to identify potentially affected deployments:

apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/TableCells.tsx (1)

88-88: LGTM! Status checks updated consistently.

Both isComplete and isRollingOut checks have been updated to use JobStatus.Successful, maintaining consistency with the new status terminology.

Also applies to: 96-96

apps/webservice/src/app/[workspaceSlug]/(app)/_components/DailyJobsChart.tsx (1)

35-35: LGTM! Chart display updated consistently.

The status mappings in both statusColors and statusLabels have been updated to use JobStatus.Successful, maintaining visual consistency in the chart display.

Also applies to: 45-45

packages/db/src/schema/job.ts (1)

54-54: Verify validators package update.

Since this is a source of truth for job statuses, ensure that @ctrlplane/validators/src/jobs/index.ts has been updated to reflect this change.

✅ Verification successful

Validators package is already correctly configured

The validators package already uses "successful" as the job status value in both:

  • packages/validators/src/jobs/index.ts: JobStatus.Successful = "successful"
  • packages/validators/src/jobs/conditions/status-condition.ts: includes "successful" in the status enum
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if the validators package has been updated with the new status.
# Test: Search for both old and new status values in the validators package.
# Expect: Only "successful" should be found, not "completed"

rg -i "completed.*status" packages/validators/src/jobs/
rg -i "successful.*status" packages/validators/src/jobs/

Length of output: 116


Script:

#!/bin/bash
# First, verify the validators package location and structure
fd validators
fd . packages/validators/src/jobs/ || true

# Then, look for any job-related files
rg -l "job.*status|status.*job" || true

# Use ast-grep to find status-related code patterns
ast-grep --pattern 'status = $_' || true
ast-grep --pattern 'status: $_' || true

Length of output: 6754


Script:

#!/bin/bash
# Check the content of the main jobs files in validators package
cat packages/validators/src/jobs/index.ts
cat packages/validators/src/jobs/conditions/status-condition.ts

# Broader search for status-related definitions
rg -A 5 -B 5 "status.*=|type.*Status|enum.*Status|interface.*Status" packages/validators/src/jobs/

Length of output: 7751

apps/webservice/src/app/[workspaceSlug]/(app)/_components/job-condition/RunbookJobComparisonConditionRender.tsx (1)

304-304: LGTM!

The status value has been correctly updated to use JobStatus.Successful in the dropdown menu item.

apps/webservice/src/app/[workspaceSlug]/(app)/_components/job-condition/JobComparisonConditionRender.tsx (1)

303-303: LGTM!

The status value has been correctly updated to use JobStatus.Successful in the dropdown menu item.

packages/api/src/router/deployment.ts (2)

665-665: LGTM!

The job status has been correctly updated to use JobStatus.Successful in the array of valid statuses for the query filter.


Line range hint 665-669: Verify all status checks in the codebase.

Let's ensure all similar status checks have been updated consistently across the codebase.

✅ Verification successful

Status checks are properly handled across the codebase

The codebase consistently handles job status transitions. While external integrations may use "completed", these are properly mapped to JobStatus.Successful internally, as evidenced by the webhook handler and database migrations.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining references to JobStatus.Completed
# Test: Search for "completed" in relation to job status
# Expect: No instances of JobStatus.Completed or similar patterns

# Search for completed status in TypeScript/JavaScript files
rg -g '*.{ts,tsx,js,jsx}' -i 'status.*completed|completed.*status'

# Search for completed in SQL migrations
rg -g '*.sql' -i 'completed.*status|status.*completed'

Length of output: 965

packages/db/drizzle/meta/_journal.json (1)

424-430: LGTM! Migration entry follows the established pattern.

The new migration entry is properly formatted and maintains consistency with existing entries in terms of structure and properties.

openapi.v1.json (3)

2637-2637: Status enum update aligns with PR objective.

The addition of "successful" status aligns with the PR's goal of converting from "completed" to "successful" status.


2658-2661: LGTM! New updatedAt field properly defined.

The updatedAt field is:

  • Properly typed as a date-time string
  • Added to the required fields
  • Consistent with other timestamp fields in the schema

Line range hint 2658-2667: Well-structured job lifecycle tracking.

The combination of timestamp fields (createdAt, updatedAt, startedAt, completedAt) provides comprehensive tracking of job state transitions, which is valuable for monitoring and debugging.

@@ -1,7 +1,7 @@
import { z } from "zod";

export const jobStatus = z.enum([
"completed",
"successful",
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Update required in dependent files for status value change

The change from "completed" to "successful" requires updates in:

  • packages/node-sdk/src/schema.ts: Still includes "completed" in the status union type
  • apps/webservice/src/app/api/github/webhook/workflow/handler.ts: Maps external "completed" status to JobStatus.Successful
🔗 Analysis chain

LGTM! Verify usage of updated status value.

The change from "completed" to "successful" in the zod schema is consistent with the PR objective.

Let's verify that all usages of the old status value have been updated:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining instances of "completed" status
rg -i "completed" --type ts

Length of output: 5482

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the first one I will do in a separate commit, the second one is intentional

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adityachoudhari26 Understood! Thanks for clarifying the webhook handler's intentional mapping.


✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#287
File: packages/validators/src/jobs/conditions/status-condition.ts:4-4
Timestamp: 2025-01-21T19:52:22.687Z
Learning: In apps/webservice/src/app/api/github/webhook/workflow/handler.ts, the external "completed" status is intentionally mapped to JobStatus.Successful to handle GitHub workflow status conversions.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!

Comment on lines 44 to 45
deployment.releaseJobTrigger?.job.status === "successful" &&
"bg-green-500/30 text-green-400 text-muted-foreground",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Use JobStatus enum instead of hardcoded string.

To maintain consistency and prevent potential bugs, replace the hardcoded "successful" string with JobStatus.Successful.

Apply this diff:

-                  deployment.releaseJobTrigger?.job.status === "successful" &&
+                  deployment.releaseJobTrigger?.job.status === JobStatus.Successful &&
                    "bg-green-500/30 text-green-400 text-muted-foreground",

Committable suggestion skipped: line range outside the PR's diff.

@adityachoudhari26 adityachoudhari26 merged commit be73da1 into main Jan 21, 2025
11 checks passed
@adityachoudhari26 adityachoudhari26 deleted the completed-to-success-refactor branch January 21, 2025 20:47
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.

2 participants