fix: Init deployment version refactor #368
Conversation
WalkthroughThe pull request updates the naming convention in the database schema. In the Changes
Possibly related PRs
Suggested reviewers
✨ 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: 0
🧹 Nitpick comments (1)
packages/job-dispatch/src/policy-create.ts (1)
44-44: Consider updating parameter name for clarityThe property
releaseIdstill retains its original name despite now being populated fromp.deployment_version.id. For complete consistency with the refactoring effort, consider also updating the parameter name to match the new terminology.- releaseId: p.deployment_version.id, + deploymentVersionId: p.deployment_version.id,This change would require updating the schema and related code that consumes this value.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
apps/webservice/src/app/api/v1/config/route.ts(1 hunks)apps/webservice/src/app/api/v1/job-agents/[agentId]/jobs/running/route.ts(1 hunks)apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts(1 hunks)packages/api/src/router/deployment.ts(4 hunks)packages/api/src/router/environment-policy.ts(2 hunks)packages/api/src/router/job.ts(4 hunks)packages/api/src/router/release.ts(7 hunks)packages/api/src/router/resource-stats.ts(1 hunks)packages/api/src/router/resources.ts(1 hunks)packages/auth/src/utils/rbac.ts(3 hunks)packages/job-dispatch/src/__test__/job-variables-deployment.test.ts(12 hunks)packages/job-dispatch/src/job-creation.ts(3 hunks)packages/job-dispatch/src/job-failure.ts(3 hunks)packages/job-dispatch/src/policies/concurrency-policy.ts(1 hunks)packages/job-dispatch/src/policies/gradual-rollout.ts(1 hunks)packages/job-dispatch/src/policies/release-dependency.ts(1 hunks)packages/job-dispatch/src/policies/release-sequencing.ts(2 hunks)packages/job-dispatch/src/policies/success-rate-criteria-passing.ts(1 hunks)packages/job-dispatch/src/policy-create.ts(1 hunks)packages/job-dispatch/src/release-job-trigger.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict en...
**/*.{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/resource-stats.tspackages/job-dispatch/src/policies/gradual-rollout.tsapps/webservice/src/app/api/v1/job-agents/[agentId]/jobs/running/route.tspackages/api/src/router/resources.tspackages/job-dispatch/src/policies/release-dependency.tspackages/job-dispatch/src/policies/success-rate-criteria-passing.tspackages/job-dispatch/src/policy-create.tspackages/job-dispatch/src/policies/release-sequencing.tspackages/job-dispatch/src/job-failure.tsapps/webservice/src/app/api/v1/jobs/[jobId]/route.tspackages/job-dispatch/src/policies/concurrency-policy.tspackages/job-dispatch/src/job-creation.tspackages/api/src/router/deployment.tspackages/job-dispatch/src/__test__/job-variables-deployment.test.tsapps/webservice/src/app/api/v1/config/route.tspackages/auth/src/utils/rbac.tspackages/api/src/router/environment-policy.tspackages/api/src/router/job.tspackages/api/src/router/release.tspackages/job-dispatch/src/release-job-trigger.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Lint
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
🔇 Additional comments (43)
packages/job-dispatch/src/policies/release-dependency.ts (1)
29-29: Rename aligns with broader refactoring effortThe property change from
release_dependencytodeployment_version_dependencyis part of a consistent terminology update throughout the codebase, but retains the same functionality.packages/api/src/router/resources.ts (1)
160-160:❓ Verification inconclusive
Verify consistent naming pattern
The property in the resulting object is still called
releasealthough it now takes its value fromrow.deployment_version. While this maintains the API contract, it creates an inconsistency between internal field names and returned properties.
🏁 Script executed:
#!/bin/bash # Check for other instances where deployment_version is mapped to release property rg --type=ts "release:.*deployment_version" -g "*.ts"Length of output: 718
Action: Clarify the naming rationale between internal fields and the API contract
After verifying across several endpoints, it’s clear that mapping the internal property
deployment_versionto the external API fieldreleaseis applied consistently (see files inpackages/api/src/router/: namelyresources.ts,resource-stats.ts,job.ts,deployment.ts, andrelease.ts). This uniform approach maintains the API contract, even though it introduces a naming divergence between internal data and what’s exposed externally.
- Confirm that this intentional design (using
releaseexternally while relying ondeployment_versioninternally) is documented or agreed upon.- If the mismatch risks confusing future maintainers, consider adding an inline comment or updating relevant documentation to explain the rationale behind the naming.
packages/api/src/router/resource-stats.ts (1)
105-105: Consistent with refactoring patternThe change from
row.releasetorow.deployment_versionfollows the same pattern seen in other files, maintaining the property name in the returned object while updating the source property.packages/job-dispatch/src/policies/gradual-rollout.ts (1)
100-104: LGTM: Consistent renaming from release to deployment_version.This rename correctly extracts
deployment_versionfrom the object while maintaining the local variable name asrelease. This approach minimizes changes to the function's logic while updating the underlying data structure.apps/webservice/src/app/api/v1/job-agents/[agentId]/jobs/running/route.ts (1)
62-67: LGTM: Consistent rename pattern applied correctly.The changes correctly update all references from
releasetodeployment_version, maintaining the same object structure in the response while updating the source of the data.apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts (1)
105-107: LGTM: Consistent rename from release to deployment_version.The condition and object creation have been properly updated to use
deployment_versionwhile maintaining the variable name asreleaseto minimize changes to downstream code.packages/job-dispatch/src/job-failure.ts (3)
32-33: LGTM: Correctly updated schema reference.The query condition now properly references
jobInfo.deployment_version.idinstead ofjobInfo.release.id.
51-52: LGTM: Function parameter updated correctly.The parameter passed to the
releasesmethod has been properly updated to usedeployment_version.id.
68-69: LGTM: Terminology updated in log message.The log message has been updated to use "deployment version" terminology, maintaining consistency with the schema changes.
packages/job-dispatch/src/policies/concurrency-policy.ts (2)
46-49: Naming changed from "release" to "deployment_version"The property access has been updated from
r.release.deploymentIdtor.deployment_version.deploymentIdto reflect the standardized naming convention being adopted across the codebase.
51-51: Naming changed from "release" to "deployment_version"The property access has been updated from
groupedTriggers[0]!.release.deploymentIdtogroupedTriggers[0]!.deployment_version.deploymentIdto maintain consistency with the renamed entity.packages/job-dispatch/src/policies/success-rate-criteria-passing.ts (1)
94-94: Updated parameter to use new naming conventionThe parameter passed to
isSuccessCriteriaPassinghas been changed frompolicy.releasetopolicy.deployment_versionto align with the standardized naming convention in the database schema.packages/api/src/router/environment-policy.ts (2)
160-160: Updated property access to use new table nameThe mapping has been changed from
r.release_channeltor.deployment_version_channelto reflect the updated table name in the database schema.
199-199: Updated property access to use new table nameThe mapping has been changed from
r.release_channeltor.deployment_version_channelto reflect the updated table name in the database schema, maintaining consistency with the first instance of this change.apps/webservice/src/app/api/v1/config/route.ts (1)
177-179: Updated property mapping to use new entity nameThe mapping has been changed from
(d) => d.releaseto(d) => d.deployment_versionto align with the standardized naming convention being used across the codebase.packages/api/src/router/release.ts (7)
108-108: Naming convention change:The update from
r.release.idtor.deployment_version.idaligns with the PR's objective to refactor naming conventions.
110-113: Consistent renaming from "release" to "deployment_version":The property access and map operation have been correctly updated to use the new naming convention, maintaining the same functionality while standardizing terminology.
153-158: Naming convention update maintained throughout groupBy operation:The groupBy and mapping operations have been consistently updated to use the new "deployment_version" naming pattern, preserving functionality.
292-295: Consistent naming in environment group operations:The environment grouping and mapping correctly applies the naming convention update from "release" to "deployment_version".
560-564: Channel property name standardization:The property check and filter access have been properly updated to use "deployment_version_channel" instead of "release_channel".
582-583: Object property access standardization:All instances of accessing the release object have been consistently updated to use "deployment_version" throughout the return statements.
Also applies to: 609-610
492-492: Nullable property access updated:The optional chaining property access has been updated to use "deployment_version" instead of "release", maintaining consistency in the naming convention change.
packages/job-dispatch/src/job-creation.ts (3)
95-101: Updated policy criteria check:The dependency trigger criteria has been correctly updated to check against
deployment_version.idinstead ofrelease.id, aligning with the naming convention changes.
103-108: Concurrency requirement check updated:The concurrency requirement check now properly uses
deployment_version.deploymentIdinstead ofrelease.deploymentIdto maintain functionality with the new naming.
114-118: Job finish waiting check updated:The condition for checking if a job is waiting on another job to finish now correctly uses
deployment_version.idfor comparison.packages/job-dispatch/src/release-job-trigger.ts (2)
177-179: Added backward compatibility for property access:The code now intelligently checks for the presence of either "deployment_version" or "release" property, providing smoother transition during the renaming refactor.
199-203: Updated release lookup logic:The find operation now correctly looks for a matching release using
deployment_version.idinstead ofrelease.id, consistent with the naming convention change.packages/job-dispatch/src/policies/release-sequencing.ts (2)
73-80: Updated max release comparison and filtering:The comparison logic for determining the maximum release and the subsequent filtering has been updated to use
deployment_versionproperties instead ofrelease, maintaining the same functionality with the new naming.
162-164: Updated release comparison conditions:The condition for determining if a release is latest and active now correctly compares against
deployment_version.idanddeployment_version.createdAtinstead of the previousreleaseproperties.packages/api/src/router/deployment.ts (4)
405-405: Property reference updated to match schema changesThe property reference has been changed from
row.releasetorow.deployment_versionto align with the database schema renaming, while maintaining the same property name in the returned object.
536-536: Property reference updated to match schema changesThe property reference has been changed from
row.release_channeltorow.deployment_version_channelto align with the database schema renaming while maintaining the same returned data structure.
568-568: Property reference updated to match schema changesThe property reference has been changed from
row.release_channeltorow.deployment_version_channelto align with the database schema renaming in the.map()function, ensuring the API returns the correct data structure.
648-648: Property reference updated to match schema changesThe property reference has been changed from
row.releasetorow.deployment_versionto align with the database schema renaming while preserving the property name in the returned object.packages/auth/src/utils/rbac.ts (3)
101-103: Property references updated to match schema changesThe property references for both "release" and "deployment" types have been updated to use
result.deployment_version.idinstead ofresult.release.idto align with the database schema renaming.
119-123: Property reference updated for releaseChannel typeThe property reference for the "releaseChannel" type has been updated to use
result.deployment_version_channel.idinstead ofresult.release_channel.idto align with the database schema renaming while maintaining the same scope type.
369-369: Property reference updated in job scopesThe property reference for the "release" type in job scopes has been updated to use
result.deployment_version.idinstead ofresult.release.idto align with the database schema renaming while maintaining the same scope type.packages/job-dispatch/src/__test__/job-variables-deployment.test.ts (3)
72-72: Variable name updated to match schema changesThe variable name has been changed from
releasetodeployment_versionwhile maintaining the same type and functionality, aligning with the schema renaming.
106-108: Type definition updated to match schema changesThe type definition has been updated to use
deployment_versioninstead ofreleaseto align with the database schema renaming while maintaining the same underlying schema type.
126-127: Variable references updated throughout test casesAll references to the
releasevariable have been updated to usedeployment_versionthroughout the test cases, ensuring consistency with the renamed variable while maintaining the same test functionality.Also applies to: 164-165, 199-200, 234-235, 304-305, 373-374, 440-441, 601-602
packages/api/src/router/job.ts (4)
195-198: Property reference updated while preserving structureThe property construction has been updated to spread
v[0]!.deployment_versioninstead ofv[0]!.releasewhile maintaining the same structure in the returned object, aligning with the database schema renaming.
426-431: Added mapping for property reference consistencyA
.then()clause has been added to map the result rows and set thereleaseproperty torow.deployment_version, ensuring consistency with the database schema renaming while maintaining the same API response structure.
486-491: Added mapping for property reference consistencyA
.then()clause has been added to map the result rows and set thereleaseproperty torow.deployment_versionin thebyIdquery, ensuring consistency with the database schema renaming while maintaining the same API response structure.
786-786: Property reference updated while preserving structureThe property construction has been updated to spread
t.deployment_versioninstead oft.releasein thebyResourceIdquery, aligning with the database schema renaming while maintaining the same API response structure.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
packages/node-sdk/src/index.ts (2)
98-101: 🛠️ Refactor suggestionPotential null reference when accessing
this.agent.id.The
next()method is usingthis.agent.idwithout first verifying thatthis.agentis not null. Ifget()hasn't been called or failed, this could cause a runtime error.async next() { + const agent = await this.get(); + if (!agent) throw new Error("Agent not found"); const { data } = await this.client.GET( "/v1/job-agents/{agentId}/queue/next", - { params: { path: { agentId: this.agent.id } } }, + { params: { path: { agentId: agent.id } } }, ); return data.jobs.map((job) => new Job(job, this.client)) ?? []; }
105-111: 🛠️ Refactor suggestionSimilar null reference issue in the
running()method.Like in the
next()method,this.agent.idis being accessed without checking ifthis.agentis null.async running() { + const agent = await this.get(); + if (!agent) throw new Error("Agent not found"); const { data } = await this.client.GET( "/v1/job-agents/{agentId}/jobs/running", - { params: { path: { agentId: this.agent.id } } }, + { params: { path: { agentId: agent.id } } }, ); return data.map((job) => new Job(job, this.client)) ?? []; }
🧹 Nitpick comments (1)
integrations/kubernetes-job-agent/src/index.ts (1)
97-97:❓ Verification inconclusive
Status value standardized from "failed" to "failure"
This change standardizes the error status naming convention from "failed" to "failure", which aligns with the overall deployment version refactoring mentioned in the PR objectives. Make sure this status value change is consistent with how other components in the system interpret job statuses.
Run the following script to verify that any consumers of this status value are updated accordingly:
🏁 Script executed:
#!/bin/bash # Check for other occurrences of "failed" status that might need to be updated rg -A 2 -B 2 "status: \"failed\"" --type ts # Check for places that might be consuming this status value rg -i "job.*status.*== \"failed\"" --type ts # Also check for places that might be consuming the new status value rg -i "job.*status.*== \"failure\"" --type tsLength of output: 134
Action Required: Verify Job Status Standardization Consistency
The change in integrations/kubernetes-job-agent/src/index.ts at line 97—updating the status from
"failed"to"failure"—appears correct based on the intent to standardize status naming. However, the verification script did not return any results for either"failed"or"failure", which might indicate that:
- There are no remaining references to the old
"failed"status, or- The search did not cover all scenarios, requiring manual verification.
Please ensure that all consumers or comparisons of job status values across the codebase have been updated accordingly. Manual checks of related components that process job statuses are recommended.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
integrations/kubernetes-job-agent/src/index.ts(1 hunks)integrations/kubernetes-job-agent/src/k8s.ts(1 hunks)packages/node-sdk/src/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict en...
**/*.{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.
integrations/kubernetes-job-agent/src/k8s.tsintegrations/kubernetes-job-agent/src/index.tspackages/node-sdk/src/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (2)
integrations/kubernetes-job-agent/src/k8s.ts (1)
71-71: Status terminology update looks good.The change from "completed" to "successful" provides a more precise description of the job's state when it has executed successfully. This aligns with the broader refactoring from "release" to "deployment version" terminology across the codebase.
packages/node-sdk/src/index.ts (1)
81-83: Correctly aligned type with the actual API operation.The type declaration change from
updateJobAgenttoupsertJobAgentproperly reflects the API operation being used in theget()method, which performs a PATCH request to/v1/job-agents/name.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/node-sdk/src/schema.ts (1)
251-278: 🛠️ Refactor suggestionAdding release endpoints
Defines"/v1/releases/{releaseId}"→updateReleaseand"/v1/releases"→upsertRelease. If the broader codebase is shifting toward “deployment_version” naming, consider updating these references for consistency.
🧹 Nitpick comments (1)
packages/node-sdk/src/schema.ts (1)
949-1068: createEnvironment returns 200
This is slightly inconsistent with many creation endpoints returning 201. If consistent with your API style, no issue; otherwise, consider standardizing response codes for newly created resources.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/node-sdk/src/schema.ts(29 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict en...
**/*.{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/node-sdk/src/schema.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (17)
packages/node-sdk/src/schema.ts (17)
7-24: New path for retrieving & deleting deployments
These lines add support for a GET (getDeployment) and DELETE (deleteDeployment) operation on/v1/deployments/{deploymentId}. The structure and use of standard HTTP status codes (200/404) look good.
42-58: New path for creating deployments
The POST operation on/v1/deploymentscleanly handles creation viacreateDeployment, returning 201 for success and 409 on conflict. This aligns well with typical REST patterns.
102-102: Rename from getAgentRunningJob → getAgentRunningJobs
Renaming is clearer and suggests multiple jobs may be returned. Implementation looks consistent.
162-162: Upserting job agent via PATCH
ChangingupdateJobAgenttoupsertJobAgentclarifies the intent to either update or create. Confirm that using PATCH meets your conventions for upserts (some might prefer PUT).
713-745: Operation getDeployment
Returning 404 if not found and 200 for success is standard practice. Looks good overall.
746-789: Operation deleteDeployment
This block neatly covers success (200), not found (404), and server error (500). Good error coverage.
849-914: Handling createDeployment request & responses
All fields (systemId, name, etc.) align well with typical creation flows. The 201, 409, and 500 responses are well-defined. Consider verifying that slugs are validated for uniqueness in your backend.
1140-1163: getNextJobs endpoint
Retrieves the next available jobs with a 200 response. Straightforward and follows typical patterns for job-dispatch.
1287-1287: Optional job status update
Allowing a status override for the job is sensible. No concerns with adding it as an optional field.
1626-1644: upsertRelease request body
Permits optional jobAgentConfig, status, message, and metadata—this matches “upsert” semantics. Implementation looks fine.
2004-2015: getSystem returns system + child resources
Returning environments and deployments in the same response is efficient for client usage. No issues found.
2017-2065: deleteSystem
Clean handling of 200 (success), 404 (not found), and 500 (internal server error). Aligned with typical REST conventions.
2066-2128: updateSystem
Supports partial updates through request body fields. The responses cover success, not found, and error states thoroughly.
2129-2192: createSystem
Uses 201 for successful creation and includes validation errors (400) and server errors (500). Follows standard patterns.
2194-2236: getWorkspace
Correctly returns 200, 404, and 500 states. No additional concerns.
2294-2329: getResourcesByFilter
Provides flexible resource filtering and uses 200 on success, 400 for invalid filter. Straightforward and well-scoped.
2467-2510: getWorkspaceBySlug
Endpoint consistently follows the same error/success (200/404/500) pattern. Implementation appears sound.
Summary by CodeRabbit
These updates enhance clarity in how version information is presented without altering system functionality.