fix: Make release channels only configurable for the policy #298
Conversation
WalkthroughThe changes refactor the handling of release channels and filters across the codebase. On the front end, components such as the deployment drawer and release channel forms now extract and pass filter values more explicitly, with obsolete UI components removed. The back-end API routes and database schema have been updated to remove direct dependencies on environment release channels in favor of policy-based management. Job dispatch logic and related hooks have been streamlined to use unified release channel data, simplifying query parameters and eliminating redundant logic. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant D as Deployment Drawer
participant H as Release Channel Hook
participant API as API Endpoint
participant DB as Database
U->>D: Interact with deployment UI
D->>H: Request release channel data
H->>API: Query policy-based release channel info
API->>DB: Fetch release channel filter & status
DB-->>API: Return channel details
API-->>H: Pass back rcId, filter, and status
H-->>D: Provide updated channel information
D->>D: Refresh UI with new release channel state
Possibly related PRs
Poem
Tip 🌐 Web search-backed reviews and chat
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
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? 🪧 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 (2)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/release-channel-drawer/Usage.tsx (1)
52-56: Ternary condition is logically correct.
As a nitpick, you could shorten the condition or use optional chaining if desired. However, the current approach is perfectly fine.apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/EnvironmentNode.tsx (1)
143-144: LGTM! Consider adding error handling.The refactoring improves code organization by:
- Using a dedicated hook for release channel logic
- Simplifying state management
- Using clear variable names
Consider adding error handling for potential hook failures.
- const { isPassingReleaseChannel, releaseChannelId, loading } = - useReleaseChannel(deploymentId, environmentId, releaseVersion); + const { isPassingReleaseChannel, releaseChannelId, loading, error } = + useReleaseChannel(deploymentId, environmentId, releaseVersion); + + if (error) { + return ( + <div className="flex items-center gap-2"> + <Failing /> Failed to load release channel + </div> + ); + }Also applies to: 149-169
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/deployment-resource-drawer/DeploymentResourceDrawer.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/ReleaseChannels.tsx(0 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/policy-override/useOverridePolicy.ts(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy-form-components/ReleaseChannels.tsx(3 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/release-channel-drawer/Usage.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/EnvironmentNode.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/useReleaseChannel.ts(2 hunks)apps/webservice/src/app/api/v1/environments/[environmentId]/route.ts(1 hunks)apps/webservice/src/app/api/v1/openapi.ts(0 hunks)apps/webservice/src/app/api/v1/systems/[systemId]/environments/[name]/route.ts(0 hunks)packages/api/src/router/deployment.ts(0 hunks)packages/api/src/router/environment.ts(0 hunks)packages/api/src/router/release.ts(2 hunks)packages/db/drizzle/0067_easy_lady_ursula.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/create-env.ts(2 hunks)packages/db/src/schema/environment-relations.ts(0 hunks)packages/db/src/schema/release-channel-relations.ts(1 hunks)packages/db/src/schema/release-channel.ts(1 hunks)packages/job-dispatch/src/environment-creation.ts(2 hunks)packages/job-dispatch/src/environment-release-channel-update.ts(2 hunks)packages/job-dispatch/src/policies/release-string-check.ts(3 hunks)packages/job-dispatch/src/resource/dispatch-resource.ts(1 hunks)
💤 Files with no reviewable changes (6)
- apps/webservice/src/app/api/v1/systems/[systemId]/environments/[name]/route.ts
- apps/webservice/src/app/api/v1/openapi.ts
- packages/db/src/schema/environment-relations.ts
- packages/api/src/router/environment.ts
- packages/api/src/router/deployment.ts
- apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/ReleaseChannels.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/db/drizzle/0067_easy_lady_ursula.sql
🧰 Additional context used
📓 Path-based instructions (15)
apps/webservice/src/app/api/v1/environments/[environmentId]/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.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/policy-override/useOverridePolicy.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/release-channel-relations.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/deployment-resource-drawer/DeploymentResourceDrawer.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/policy-form-components/ReleaseChannels.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/release.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/create-env.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/environment-creation.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-string-check.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/environment-release-channel-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/job-dispatch/src/resource/dispatch-resource.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/release-channel.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]/EnvironmentNode.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/[deploymentSlug]/releases/[versionId]/useReleaseChannel.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/release-channel-drawer/Usage.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.
📓 Learnings (2)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/deployment-resource-drawer/DeploymentResourceDrawer.tsx (1)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#188
File: apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/Usage.tsx:42-44
Timestamp: 2024-11-12T09:55:38.456Z
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.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/release-channel-drawer/Usage.tsx (1)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#188
File: apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/Usage.tsx:42-44
Timestamp: 2024-11-12T09:55:38.456Z
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.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (35)
packages/api/src/router/release.ts (3)
282-282: LGTM!The formatting adjustment improves code readability while maintaining the same functionality.
302-304: LGTM! Clean refactoring of release channel property extraction.The changes improve code clarity through:
- Direct destructuring of properties
- Clean null handling with the nullish coalescing operator
- Early return pattern for undefined filters
312-312: LGTM! Simplified release matching condition.The change aligns with the policy-based release channel management approach, reducing complexity while maintaining functionality.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/release-channel-drawer/Usage.tsx (7)
1-2: No concerns found for these imports.
8-8: No concerns found for this import.
10-12: Good use of NonNullable.
This type definition clearly ensures thatusageis not null, which enhances type safety.
15-15: UsinguseQueryParamsfor parameter management is appropriate here.
No issues found with this approach, and it keeps the component logic straightforward.
17-22: ThesetEnvironmentIdAndTabfunction logic looks correct.
This ensures the query parameters are properly set for the environment drawer.
24-29: ThesetPolicyIdAndTabfunction logic appears correct.
No issues found with setting the query parameters for the policy drawer.
35-35: Destructuringpoliciesfromusageis straightforward and clear.apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-drawer/policy-override/useOverridePolicy.ts (3)
5-17: LGTM! Well-structured cache invalidation.The hook effectively groups related cache invalidations and follows a logical order from specific to general queries.
19-28: LGTM! Clean hook implementation.The hook follows React patterns well with proper TypeScript types and good separation of concerns.
Also applies to: 33-37
30-31: LGTM! Concise Promise chain implementation.The simplified Promise chain maintains the same functionality while following the coding guidelines for error handling.
Let's verify how errors are handled up the call chain:
✅ Verification successful
LGTM! The error propagation is intact, and the concise Promise chain in the onUpdate function correctly passes errors up the call chain.
- Error handling in components (e.g., in
apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy-form-components/DeploymentControl.tsx) demonstrates that errors from onUpdate calls are properly caught.- The overall design complies with our coding guidelines with consistent error management across usage sites.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for error handling of onUpdate calls rg -A 5 "onUpdate\(" apps/webservice/src/appLength of output: 6600
packages/db/src/schema/release-channel-relations.ts (1)
3-4: LGTM! The relations are correctly defined.The changes align with the PR objective by removing environment-level release channel relations and maintaining policy-level relations. The remaining relations correctly define:
- Many-to-many relationship between release channels and environment policies
- One-to-many relationship between environment policies and their release channels
Also applies to: 7-12, 14-26
packages/db/src/schema/release-channel.ts (1)
4-6: LGTM! The schema is correctly defined with proper constraints.The changes align with the PR objective by:
- Removing environment-level release channel configuration
- Maintaining policy-level release channel configuration with proper foreign key constraints
- Ensuring data integrity through unique indexes on policy-channel and policy-deployment pairs
Also applies to: 8-26, 28-30
packages/db/src/create-env.ts (1)
9-20: LGTM! The release channel creation is correctly updated.The changes align with the PR objective by:
- Shifting release channel configuration from environment to policy level
- Correctly handling the creation of policy-level release channels
- Maintaining proper data consistency during environment creation
Also applies to: 51-52
packages/job-dispatch/src/environment-creation.ts (1)
33-35: LGTM! The job creation logic is correctly updated.The changes align with the PR objective by:
- Accessing release channels through the policy instead of directly from the environment
- Correctly handling policy-level release channels in job creation
- Maintaining proper data access patterns
Also applies to: 50-53
apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy-form-components/ReleaseChannels.tsx (4)
6-6: LGTM!The Button import is correctly added and aligns with the new Clear button functionality.
41-42: LGTM!The onChange function has been simplified to directly handle null values, improving code clarity.
77-83: LGTM!The Clear button is well-implemented with appropriate:
- Variant styling
- onClick handler for clearing the selection
- Disabled state when no release channel is selected
88-93: LGTM!The ReleaseChannelProps type declaration is well-structured and appropriately placed near its usage.
packages/job-dispatch/src/environment-release-channel-update.ts (2)
191-191: LGTM!The query has been appropriately simplified to focus on policy ID, aligning with the objective of making release channels policy-specific.
204-205: LGTM!The environment object destructuring has been appropriately simplified to only extract necessary properties, aligning with the policy-based release channel management.
Also applies to: 209-209
packages/job-dispatch/src/resource/dispatch-resource.ts (1)
66-68: LGTM!The changes correctly:
- Simplify environment destructuring
- Update release channel access to use policy
- Maintain proper error handling through optional chaining
Also applies to: 70-70, 74-74
packages/db/drizzle/meta/_journal.json (1)
474-480: LGTM!The new migration entry is correctly formatted and maintains consistency with existing entries.
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/useReleaseChannel.ts (3)
23-26: LGTM! Clean and concise release channel extraction.The code effectively extracts the release channel ID and filter from the policy, with proper null handling.
34-38: LGTM! Well-structured filter composition.The release filter construction using
_.compactelegantly handles both version and policy filters.
55-55: LGTM! Enhanced return value.The addition of
releaseChannelIdto the return value improves the hook's utility.apps/webservice/src/app/api/v1/environments/[environmentId]/route.ts (1)
23-23: LGTM! Simplified environment query.The removal of
releaseChannelsfrom thewithclause aligns with the architectural shift to policy-based release channel management.packages/job-dispatch/src/policies/release-string-check.ts (3)
51-64: LGTM! Improved data transformation.The chain of transformations using lodash is clean and efficient, properly grouping and mapping the data.
79-84: LGTM! Simplified release channel handling.The code effectively handles the release channel filter with proper null checks.
42-45: Verify the impact of switching to inner join.The change from left join to inner join with
environmentPolicymight exclude environments without policies.✅ Verification successful
The inner join change is appropriate given current usage.
- The query now returns only environments that have an associated policy, which matches the downstream logic that immediately dereferences
environment_policywithout null checks.- No evidence was found that environments without policies are valid or required in this context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for environments that might be affected by the join change ast-grep --pattern 'schema.environment.find($$$)' | rg -A 5 'where.*policyId.*null'Length of output: 85
Script:
#!/bin/bash # Print the context around the inner join in release-string-check.ts to check the usage context sed -n '40,60p' packages/job-dispatch/src/policies/release-string-check.tsLength of output: 783
Script:
#!/bin/bash # Search for the schema definition of "environment" to check if policyId is non-nullable, # which would justify using an inner join. rg "environment.*policyId" -n packages/job-dispatch/src/schema*Length of output: 141
apps/webservice/src/app/[workspaceSlug]/(app)/_components/deployment-resource-drawer/DeploymentResourceDrawer.tsx (2)
49-52: LGTM! Clean release filter extraction.The destructuring with default empty object pattern effectively handles the case when no matching release channel is found.
80-80: LGTM! Explicit filter handling.The use of nullish coalescing with
undefinedensures proper filter behavior in the query.apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/EnvironmentNode.tsx (1)
32-32: LGTM! Good architectural change.Moving release channel logic to a dedicated hook improves code organization and maintainability.
Summary by CodeRabbit
New Features
Refactor
Chores