Skip to content

Conversation

@adityachoudhari26
Copy link
Contributor

@adityachoudhari26 adityachoudhari26 commented Jan 18, 2025

Summary by CodeRabbit

  • New Features

    • Introduced policy-based approval system, replacing environment-based approvals
    • Enhanced approval tracking with linked environments and policy-specific checks
  • Database Changes

    • Created new environment_policy_approval table
    • Replaced environment_approval table with policy-focused approval tracking
  • UI Updates

    • Updated approval dialogs to show release version and linked environments
    • Modified flow diagrams and policy node rendering to support new approval model
  • Backend Modifications

    • Refactored API routes and routers to support policy-based approval workflow
    • Updated database queries to use policy ID instead of environment ID

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 18, 2025

Walkthrough

The pull request introduces a comprehensive shift from environment-based approvals to policy-based approvals across multiple components of the system. The changes span database schema modifications, API routes, frontend components, and job dispatch logic. The primary transformation involves replacing environmentApproval with environmentPolicyApproval, introducing a more granular and policy-centric approach to managing release and deployment approvals.

Changes

File Path Change Summary
apps/jobs/src/policy-checker/index.ts Updated approval gate logic to use environmentPolicyApproval
apps/webservice/src/**/ApprovalCheck.tsx Refactored approval dialog and check components to work with policy-based approvals
apps/webservice/src/**/EnvironmentApprovalRow.tsx Modified approval row to use policy approvals and display approval status
apps/webservice/src/**/EnvironmentNode.tsx Removed ApprovalCheck component
packages/api/src/router/environment-approval.ts Restructured approval router to use policy-based approvals
packages/db/drizzle/0055_wandering_rogue.sql Created new environment_policy_approval table and dropped old environment_approval table

Sequence Diagram

sequenceDiagram
    participant User
    participant PolicyApproval
    participant Release
    participant Environment

    User->>PolicyApproval: Initiate Approval
    PolicyApproval->>Release: Check Release Details
    PolicyApproval->>Environment: Validate Linked Environments
    PolicyApproval-->>User: Approve/Reject Release
Loading

Possibly related PRs

Suggested reviewers

  • jsbroks

Poem

🐰 A Rabbit's Ode to Policy Approval 🚀

From env to policy, we dance and leap,
Approvals now have a structure more deep
Migrations flow, schemas rearrange
With CodeRabbit's touch, we welcome change!
Hop, hop, hurray for policy's new way! 🎉

Finishing Touches

  • 📝 Generate Docstrings (Beta)

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: 1

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

53-53: LGTM! Consider memoizing for large environment lists.

The addition of linkedEnvironments aligns well with the policy-based approval system. The implementation is clean and maintains immutability.

If the environments list is expected to be large, consider memoizing the filtered results:

const linkedEnvironments = useMemo(
  () => envs.filter((e) => e.policyId === policy.id),
  [envs, policy.id]
);
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ResourceReleaseTable.tsx (1)

83-89: Consider memoizing linkedEnvironments.

The environment policy query and linkedEnvironments setup looks good. However, since this value is used in child components, consider memoizing it to prevent unnecessary re-renders.

-  const linkedEnvironments = environmentPolicyQ.data?.environments ?? [];
+  const linkedEnvironments = React.useMemo(
+    () => environmentPolicyQ.data?.environments ?? [],
+    [environmentPolicyQ.data?.environments]
+  );
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ApprovalCheck.tsx (2)

34-38: Add error handling for mutation promises

Consider adding error handling to the approve and reject mutation promises to handle any potential errors during the approval or rejection process.

Apply this diff to include error handling:

  const onApprove = () =>
    approve
      .mutateAsync({ releaseId, policyId })
      .then(() => router.refresh())
      .then(() => setOpen(false))
+     .catch((error) => {
+       // Handle error, e.g., display a notification
+       console.error("Approval failed:", error);
+     });

  const onReject = () =>
    reject
      .mutateAsync({ releaseId, policyId })
      .then(() => router.refresh())
      .then(() => setOpen(false))
+     .catch((error) => {
+       // Handle error, e.g., display a notification
+       console.error("Rejection failed:", error);
+     });

Also applies to: 39-45


46-46: Move useRouter hook above function definitions

The useRouter hook should be called before any functions that use it to comply with React's Rules of Hooks and prevent potential issues with hook ordering.

Apply this diff to rearrange the code:

+ const router = useRouter();
  const [open, setOpen] = useState(false);
  const approve = api.environment.policy.approval.approve.useMutation();
  const reject = api.environment.policy.approval.reject.useMutation();
- const router = useRouter();
  const releaseId = release.id;
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between f0b8b9e and 1e099fe.

📒 Files selected for processing (16)
  • apps/jobs/src/policy-checker/index.ts (2 hunks)
  • apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ApprovalCheck.tsx (4 hunks)
  • apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/EnvironmentApprovalRow.tsx (1 hunks)
  • apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/EnvironmentNode.tsx (0 hunks)
  • apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowDiagram.tsx (1 hunks)
  • apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowPolicyNode.tsx (4 hunks)
  • apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ResourceReleaseTable.tsx (3 hunks)
  • apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts (3 hunks)
  • packages/api/src/router/environment-approval.ts (3 hunks)
  • packages/api/src/router/environment-policy.ts (4 hunks)
  • packages/api/src/router/environment.ts (0 hunks)
  • packages/db/drizzle/0055_wandering_rogue.sql (1 hunks)
  • packages/db/drizzle/meta/_journal.json (1 hunks)
  • packages/db/src/schema/environment.ts (1 hunks)
  • packages/job-dispatch/src/policies/manual-approval.ts (2 hunks)
  • packages/job-dispatch/src/policy-create.ts (3 hunks)
💤 Files with no reviewable changes (2)
  • packages/api/src/router/environment.ts
  • apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/EnvironmentNode.tsx
🧰 Additional context used
📓 Path-based instructions (12)
packages/job-dispatch/src/policy-create.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]/FlowDiagram.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/policies/manual-approval.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/jobs/src/policy-checker/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/[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.

packages/api/src/router/environment-policy.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]/EnvironmentApprovalRow.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/jobs/[jobId]/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)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ApprovalCheck.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/db/src/schema/environment.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/api/src/router/environment-approval.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]/ResourceReleaseTable.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.

⏰ 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 (31)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowDiagram.tsx (1)

53-53: Verify linkedEnvironments usage in dependent components.

Let's ensure that all components consuming this new property are properly updated.

✅ Verification successful

All components are properly consuming the linkedEnvironments property

The linkedEnvironments property is correctly implemented across all dependent components:

  • FlowPolicyNode (consumer)
  • ApprovalCheck (consumer)
  • ResourceReleaseTable (consumer)
  • EnvironmentApprovalRow (consumer)
  • FlowDiagram (provider)
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for components that might need the linkedEnvironments property
rg -t typescript -t typescriptreact "PolicyNode|FlowPolicyNode" --glob "!{node_modules,dist}/**"

# Search for potential PropTypes or interface definitions that might need updating
ast-grep --pattern 'interface $_ {
  $$$
  linkedEnvironments
  $$$
}'

Length of output: 206


Script:

#!/bin/bash
# Search for PolicyNode components and linkedEnvironments usage
rg -t tsx -t ts "PolicyNode|FlowPolicyNode" -B 2 -A 2 --glob "!{node_modules,dist}/**"
rg -t tsx -t ts "linkedEnvironments" -B 2 -A 2 --glob "!{node_modules,dist}/**"

# Search for interface definitions and property usage
ast-grep --pattern 'interface $_ {
  $$$
  linkedEnvironments
  $$$
}'

ast-grep --pattern '$_.linkedEnvironments'

Length of output: 34121


Script:

#!/bin/bash
# Search for FlowPolicyNode imports and usage
rg "FlowPolicyNode" --type-add "ts:*.{ts,tsx}" --type ts -B 2 -A 2

# Search for linkedEnvironments in type definitions and interfaces
rg "linkedEnvironments" --type-add "ts:*.{ts,tsx}" --type ts -B 2 -A 2

# Search for type definitions using ast-grep
ast-grep --pattern 'type $_ = {
  $$$
  linkedEnvironments
  $$$
}'

ast-grep --pattern 'interface $_ {
  $$$
  linkedEnvironments
  $$$
}'

Length of output: 12989

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

155-156: Verify EnvironmentApprovalRow component compatibility.

The component now receives release and linkedEnvironments instead of environment. Ensure that the EnvironmentApprovalRow component has been updated to handle these new props correctly.

✅ Verification successful

Component implementation verified and compatible

The EnvironmentApprovalRow component has been properly updated to handle the new props structure. It correctly uses release and linkedEnvironments props along with the policy-based approval system. The old environment prop has been completely removed.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify EnvironmentApprovalRow component changes

# Check for any remaining references to the old environment prop
ast-grep --pattern 'EnvironmentApprovalRow $props environment='

# Find the component definition to verify prop types
ast-grep --pattern 'type EnvironmentApprovalRowProps = {'

Length of output: 125


Script:

#!/bin/bash
# Find EnvironmentApprovalRow component file
fd EnvironmentApprovalRow

# Once found, let's examine its content
rg -B2 -A5 "EnvironmentApprovalRow.*=.*" --type typescript

# Try a different ast-grep pattern for interface/type definition
ast-grep --pattern 'interface EnvironmentApprovalRowProps'

Length of output: 328


Script:

#!/bin/bash
# Read the component file
cat "apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/EnvironmentApprovalRow.tsx"

# Also try rg without type specification as backup
rg -B2 -A5 "EnvironmentApprovalRow.*=.*" "apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/EnvironmentApprovalRow.tsx"

Length of output: 1945


65-71: Verify the policy-based approval filtering.

The change from environment-based to policy-based approval filtering looks correct. However, we should ensure that all approvals are properly matched when multiple environments share the same policy.

Run this script to verify the approval matching:

✅ Verification successful

Policy-based approval filtering is correctly implemented

The current implementation properly handles cases where multiple environments share the same policy. The filtering logic ensures that approvals are correctly matched with the environment's policy, which is the intended behavior for the policy-based approval system.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for environments sharing the same policy to ensure correct approval matching

# Find environments sharing policies
ast-grep --pattern 'environment.policyId === $policy'

# Check approval queries
ast-grep --pattern 'api.environment.policy.approval.byReleaseId'

Length of output: 341


Script:

#!/bin/bash
# Search for policy-based filtering patterns
ast-grep --pattern 'filter($$$) => $$.policyId'

# Look for policy-related components and hooks
rg -l "policyId|PolicyId" --type ts --type tsx

# Check for environment-policy relationships
ast-grep --pattern 'environment: { $$$, policyId: $$ }'

Length of output: 184


Script:

#!/bin/bash
# Search for policy-related code with corrected file type
rg -l "policyId|PolicyId" --type typescript

# Look for policy-based filtering with more flexible patterns
ast-grep --pattern 'filter((a) => a.policyId'

# Check for environment-policy relationships with broader pattern
ast-grep --pattern 'environment.policyId'

# Search for approval-related code
rg -B2 -A2 "approval" --type typescript

Length of output: 34994

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

Line range hint 22-79: ApprovalDialog component updated correctly

The changes to the ApprovalDialog component correctly implement the new props and logic for policy-based approvals. The modifications align with the intended shift and maintain component integrity.


Line range hint 81-140: ApprovalCheck component revisions are appropriate

The updates to the ApprovalCheck component effectively integrate the new policy-based approval system. The conditional rendering and status handling are well-implemented.

packages/api/src/router/environment-approval.ts (3)

Line range hint 33-70: Database queries updated appropriately to use policy-based approvals

The modifications in the queries reflect the shift to SCHEMA.environmentPolicyApproval and are correctly structured with appropriate joins and conditions.


71-107: Approve mutation correctly updated

The approve mutation now correctly uses policyId and updates the environmentPolicyApproval table accordingly. The dispatch logic for release job triggers is appropriately adjusted.


132-158: Reject mutation and status query updated correctly

The reject mutation and statusByReleasePolicyId procedure are updated to use policyId, and the raw SQL for updating job statuses is correctly constructed.

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

14-16: Props updated to reflect policy-based approvals

The EnvironmentApprovalRowProps type now includes release and linkedEnvironments, aligning with the new approval structure. These updates ensure the component receives all necessary data.


24-35: Conditional rendering of ApprovalDialog is appropriate

The component now correctly renders the ApprovalDialog when the approval status is "pending", utilizing the updated props. This enhances the user interface and aligns with the new approval flow.

packages/job-dispatch/src/policy-create.ts (2)

9-9: LGTM! Consistent schema transition to policy-based approvals.

The changes correctly implement the transition from environment-based to policy-based approvals, maintaining data integrity with proper foreign key relationships.

Also applies to: 40-46


19-19: Verify the impact of using environmentPolicy.id in selectDistinctOn.

The change from using environment.id to environmentPolicy.id in selectDistinctOn could affect the uniqueness criteria. Please ensure this doesn't lead to duplicate approvals.

✅ Verification successful

The change to use environmentPolicy.id in selectDistinctOn is correct and safe

The change aligns with the schema design where approvals are tracked at the policy level, not the environment level. The uniqueness is maintained through:

  • One-to-one relationship between environment and policy
  • Unique constraint on (policyId, releaseId) in the approvals table
  • Correct join chain ensuring policy-level grouping
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for potential duplicate approvals
ast-grep --pattern 'selectDistinctOn([$$_environment.id$$$])' | grep -v 'environmentPolicy.id'

Length of output: 94


Script:

#!/bin/bash
# Search for all selectDistinctOn usages
rg "selectDistinctOn" -A 2 -B 2

# Look for type definitions or interfaces related to environment and environmentPolicy
ast-grep --pattern 'interface $_Environment {
  $$$
}'

# Search for environment and environmentPolicy relationship
rg "environmentPolicy|environment" --type typescript -A 2 -B 2

Length of output: 1382


Script:

#!/bin/bash
# Search for environment and policy relationships in TypeScript files
rg "environmentPolicy|environment" -t ts -A 2 -B 2

# Look for schema or table definitions
rg "createTable|schema" -t ts -A 3 -B 3

# Search for type definitions related to the tables
ast-grep --pattern 'type $_Table = {
  $$$
}'

Length of output: 74536

packages/job-dispatch/src/policies/manual-approval.ts (2)

36-42: LGTM! Proper join conditions for policy approvals.

The join conditions correctly link the policy approvals with their corresponding releases and policies.


56-56: LGTM! Consistent status check.

The status check is properly updated to use the new policy approval field.

apps/jobs/src/policy-checker/index.ts (2)

16-16: LGTM! Proper approval gate condition.

The approval gate condition correctly checks the policy approval status.


32-42: LGTM! Well-structured join conditions.

The join conditions properly link policy approvals with their corresponding policies and releases.

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

2-2: LGTM! Proper type definitions.

The Environment type is properly imported and integrated into the PolicyNodeProps.

Also applies to: 26-26


105-105: LGTM! Well-structured conditional rendering.

The approval check is properly integrated with the correct props being passed to the ApprovalCheck component.

Also applies to: 116-122


124-124: LGTM! Comprehensive policy check handling.

The condition for displaying "No policy checks" properly includes all three cases: no minimum success requirement, no rollout, and no approval requirement.

packages/db/drizzle/0055_wandering_rogue.sql (3)

1-9: LGTM! Table creation and drop statements are well structured.

The migration correctly:

  • Creates the new policy-based approval table with appropriate columns and constraints
  • Drops the old environment-based approval table

28-28: LGTM! Unique index ensures data integrity.

The unique index on (policy_id, release_id) correctly ensures that there can only be one approval per policy and release combination.


10-26: Verify data migration strategy.

While the foreign key constraints are correctly defined with appropriate cascade rules, ensure there's a strategy to migrate existing approval data before dropping the old table.

Run this script to check for existing approvals that need migration:

packages/db/src/schema/environment.ts (2)

233-246: LGTM! Schema definition aligns with migration.

The environmentPolicyApproval table definition correctly matches the SQL migration with proper types and constraints.


249-251: LGTM! Type definition ensures type safety.

The exported type EnvironmentPolicyApproval correctly reflects the table structure.

packages/api/src/router/environment-policy.ts (2)

30-31: LGTM! Router composition is correct.

The approval router is properly integrated into the policy router.


Line range hint 151-182: LGTM! Query enhancement improves data access.

The addition of environment data to the policy query provides necessary context for the new policy-based approval system.

apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts (4)

16-16: LGTM! Type definition updated correctly.

The ApprovalJoinResult type properly reflects the schema changes.


20-31: LGTM! Function signature and query updated appropriately.

The getApprovalDetails function correctly uses policy-based approval:

  • Updated parameter from environmentId to policyId
  • Updated table reference to environmentPolicyApproval
  • Updated join conditions

41-44: LGTM! Response mapping updated consistently.

The mapApprovalResponse function correctly maps the new schema structure.


126-130: Verify null safety in approval check.

The approval check correctly uses optional chaining, but verify that policyId is always set when approvals are required.

Run this script to check for environments that require approvals but don't have a policy:

✅ Verification successful

Null safety in approval check is properly implemented.

The code correctly handles null cases through:

  • Optional chaining for both je.release?.id and je.environment?.policyId
  • Environments without policy automatically bypass approval requirements
  • Database schema and queries properly handle null relationships
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find environments that might require approvals but don't have a policy

ast-grep --pattern 'const environment = {
  $$$
  policyId: null,
  $$$
  approvalRequirement: "manual",
  $$$
}'

Length of output: 115


Script:

#!/bin/bash
# Find type definitions and usages related to environment, policy, and approvals

# Search for type definitions
echo "=== Type definitions ==="
ast-grep --pattern 'type $_Environment = {
  $$$
}'

echo -e "\n=== Interface definitions ==="
ast-grep --pattern 'interface $_Environment {
  $$$
}'

echo -e "\n=== Approval requirement references ==="
rg "approvalRequirement.*manual" -A 2 -B 2

echo -e "\n=== Policy ID usage patterns ==="
rg "policyId" -A 2 -B 2

Length of output: 62323

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

389-395: LGTM! The migration journal entry is well-structured.

The new entry follows the established pattern with sequential indexing, consistent versioning, and proper timestamp ordering.

Comment on lines +389 to +395
},
{
"idx": 55,
"version": "7",
"when": 1737154626633,
"tag": "0055_wandering_rogue",
"breakpoints": true
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Migration strategy requires attention before deployment

The migration shows a direct table replacement without data preservation:

  • Existing approvals in environment_approval will be lost when the table is dropped
  • No data migration script found to transfer approvals to the new environment_policy_approval table
  • Frontend and backend changes are coordinated, but deployment documentation is missing

While the code changes are well-coordinated across frontend and backend, please ensure:

  1. Create a data migration plan to preserve existing approvals
  2. Document the deployment sequence to minimize disruption
  3. Consider a phased rollout strategy to validate the new policy-based system
🔗 Analysis chain

Verify the migration strategy for the approval system changes.

This migration represents a significant architectural change from environment-based to policy-based approvals. Please ensure:

  1. Data migration plan exists for existing approvals
  2. Backward compatibility or upgrade path is documented
  3. Frontend and backend changes are coordinated for deployment

Let's verify the related migration files and changes:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Analyze the migration impact and related changes

# Check the actual migration SQL
echo "=== Migration SQL ==="
cat packages/db/drizzle/0055_wandering_rogue.sql

# Look for related approval system changes
echo -e "\n=== Related Changes ==="
rg -l "environmentApproval|environmentPolicyApproval" --type ts --type tsx

# Check for potential breaking changes in TypeScript files
echo -e "\n=== Type Changes ==="
rg -A 3 "interface.*Approval|type.*Approval" --type ts

Length of output: 3896


Script:

#!/bin/bash
# Look for migration documentation and deployment instructions

# Check README files
echo "=== README Files ==="
fd -e md -e mdx . | xargs cat

# Check for migration scripts
echo -e "\n=== Migration Scripts ==="
fd -e sh -e js -e ts -p "migrate" .

# Look for deployment docs
echo -e "\n=== Deployment Docs ==="
fd -g "*deploy*.md" -g "*migration*.md" .

Length of output: 70870

@adityachoudhari26 adityachoudhari26 merged commit 40589ff into main Jan 18, 2025
11 checks passed
@adityachoudhari26 adityachoudhari26 deleted the revert-policy-approval-stuff branch January 18, 2025 02:20
This was referenced Apr 6, 2025
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