feat(j-s): Block create subpoena on staging and production #16297
Conversation
WalkthroughThe changes in this pull request involve updates to the Changes
Possibly related PRs
Suggested labels
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
🧹 Outside diff range and nitpick comments (2)
apps/judicial-system/backend/src/app/modules/subpoena/subpoena.service.ts (1)
183-192: Approve with suggestions: Improve error handling and return valuesThe new error handling logic aligns with the PR objectives. However, consider the following improvements:
Returning
{ delivered: true }forNotImplementedExceptionmight be misleading. Consider introducing a new status to indicate that the operation was blocked intentionally.Add environment-specific checks to explicitly block subpoena creation in staging and production environments.
Here's a suggested improvement:
import { ConfigService } from '@nestjs/config'; // In the constructor constructor( // ... other dependencies private configService: ConfigService, ) {} // In the deliverSubpoenaToPolice method async deliverSubpoenaToPolice( // ... existing parameters ): Promise<DeliverResponse> { const environment = this.configService.get<string>('NODE_ENV'); if (environment === 'staging' || environment === 'production') { this.logger.info('Subpoena creation blocked in staging/production environment'); return { delivered: false, blocked: true }; } try { // ... existing try block } catch (error) { if (error instanceof NotImplementedException) { this.logger.info('Subpoena creation not implemented in this environment', error); return { delivered: false, notImplemented: true }; } else { this.logger.error('Error delivering subpoena to police', error); return { delivered: false, error: true }; } } }This approach provides more granular status information and explicitly blocks subpoena creation in staging and production environments.
apps/judicial-system/backend/src/app/modules/police/police.service.ts (1)
620-624: LGTM! Consider enhancing the error message.The addition of this check aligns well with the PR objectives to block subpoena creation in certain environments. The use of a configuration flag (
policeCreateSubpoenaApiAvailable) provides flexibility in controlling this feature across different environments.Consider enhancing the error message to include more context:
- 'Police create subpoena API not available in current environment', + `Police create subpoena API not available in ${process.env.NODE_ENV} environment`,This change would provide more specific information about which environment the API is unavailable in, potentially aiding in debugging and clarity.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
- apps/judicial-system/backend/infra/judicial-system-backend.ts (1 hunks)
- apps/judicial-system/backend/src/app/modules/police/police.config.ts (1 hunks)
- apps/judicial-system/backend/src/app/modules/police/police.service.ts (2 hunks)
- apps/judicial-system/backend/src/app/modules/subpoena/subpoena.service.ts (2 hunks)
- charts/judicial-system/values.prod.yaml (1 hunks)
- charts/judicial-system/values.staging.yaml (1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
apps/judicial-system/backend/infra/judicial-system-backend.ts (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/police/police.config.ts (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/police/police.service.ts (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/subpoena/subpoena.service.ts (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (10)
apps/judicial-system/backend/src/app/modules/police/police.config.ts (1)
17-21: Implementation looks good and aligns with PR objectives.The addition of
policeCreateSubpoenaApiAvailableproperty is well-implemented and consistent with the existing pattern in the file. It correctly checks theBLOCKED_API_INTEGRATIONenvironment variable to determine if the CREATE_SUBPOENA API should be blocked, which aligns with the PR objective of preventing subpoena creation requests in staging and production environments.A few observations:
- The implementation follows NextJS best practices for backend configuration.
- It maintains type safety by using TypeScript.
- The logic is consistent with the existing
policeCaseApiAvailableproperty.To ensure this change is properly utilized, let's verify its usage in the codebase:
✅ Verification successful
Verification Successful
The
policeCreateSubpoenaApiAvailableproperty is correctly utilized inpolice.service.ts, and all references toCREATE_SUBPOENAare appropriately managed through environment configurations. No issues were found with the implementation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of policeCreateSubpoenaApiAvailable in the codebase # Test: Search for usage of policeCreateSubpoenaApiAvailable rg --type typescript -A 5 'policeCreateSubpoenaApiAvailable' # Test: Check for any hardcoded references to CREATE_SUBPOENA that might need updating rg --type typescript 'CREATE_SUBPOENA'Length of output: 169
Script:
#!/bin/bash # Description: Verify the usage of policeCreateSubpoenaApiAvailable in the codebase # Test: Search for usage of policeCreateSubpoenaApiAvailable rg --type ts -A 5 'policeCreateSubpoenaApiAvailable' # Test: Check for any hardcoded references to CREATE_SUBPOENA that might need updating rg --type ts 'CREATE_SUBPOENA'Length of output: 1583
apps/judicial-system/backend/infra/judicial-system-backend.ts (1)
43-44: LGTM! Verify the impact on subpoena creation.The changes correctly implement the blocking of the
CREATE_SUBPOENAAPI integration in both staging and production environments, aligning with the PR objectives. This implementation should effectively prevent subpoena creation requests from being sent in these environments.To ensure the changes have the intended effect, please verify that subpoena creation is indeed blocked in the staging and production environments. Consider adding a test or manual verification step to confirm this behavior.
apps/judicial-system/backend/src/app/modules/subpoena/subpoena.service.ts (1)
11-11: LGTM: Import of NotImplementedExceptionThe addition of
NotImplementedExceptionis in line with the PR objectives and will be used to handle cases where the subpoena creation API is not available in certain environments.charts/judicial-system/values.staging.yaml (1)
108-108: LGTM! Verify the feature in staging.The addition of
CREATE_SUBPOENAto theBLOCKED_API_INTEGRATIONenvironment variable aligns with the PR objective to block subpoena creation API calls in the staging environment. This change effectively prevents the system from creating subpoenas in staging, as intended.To ensure the feature is working as expected, please verify in the staging environment that attempts to create subpoenas are indeed blocked. You can run the following command to check the environment variable:
The output should include
CREATE_SUBPOENAalong withCOURTandPOLICE_CASE.charts/judicial-system/values.prod.yaml (6)
Line range hint
237-237: Verify the need for increased CPU requestThe CPU request for
judicial-system-webhas been significantly increased from 15m to 100m. This change will affect resource allocation and potentially impact pod scheduling. Please confirm:
- The reason for this substantial increase in CPU request.
- Whether this change has been tested in a staging environment to ensure it doesn't cause any issues with pod scheduling or resource contention.
- If this increase is based on observed resource usage patterns.
To help verify the appropriateness of this change, you can run the following script to check the current CPU usage:
#!/bin/bash # This script assumes you have kubectl configured to access your cluster # and the necessary permissions to view resource usage # Get the current CPU usage for the judicial-system-web pods kubectl top pods -n judicial-system -l app=judicial-system-web
Line range hint
508-508: Clarify the purpose of the new TIME_TO_LIVE_MINUTES variableA new environment variable
TIME_TO_LIVE_MINUTES: '30'has been added to thejudicial-system-schedulerconfiguration. Please provide information on:
- The specific purpose of this variable and what it controls.
- Any potential impacts on data retention or task execution.
- Whether this value has been tested and verified to be appropriate for the production environment.
To help understand the usage of this new variable, you can run the following script:
#!/bin/bash # Search for usage of TIME_TO_LIVE_MINUTES in the codebase rg "TIME_TO_LIVE_MINUTES" --type ts --type js
Line range hint
314-316: Verify audit trail configuration changesAudit trail configuration has been added or updated for multiple services (judicial-system-digital-mailbox-api, judicial-system-robot-api). The changes include:
AUDIT_TRAIL_GROUP_NAME: 'k8s/judicial-system/audit-log'AUDIT_TRAIL_REGION: 'eu-west-1'AUDIT_TRAIL_USE_GENERIC_LOGGER: 'false'Please confirm:
- The purpose of these audit trail changes and their expected impact on logging and monitoring.
- Whether these settings are consistent across all services that require audit logging.
- If any additional configuration or infrastructure changes are needed to support these audit trail updates.
To help verify the consistency of audit trail configuration across services, you can run the following script:
#!/bin/bash # Search for audit trail configuration in the values file grep -n "AUDIT_TRAIL" charts/judicial-system/values.prod.yamlAlso applies to: 602-604
Line range hint
242-244: Review standardization of replica count settingsThe replica count settings have been standardized across multiple services (judicial-system-backend, judicial-system-digital-mailbox-api, judicial-system-message-handler, judicial-system-robot-api, judicial-system-web, judicial-system-xrd-api) to:
- default: 3
- max: 10
- min: 3
Please confirm:
- The rationale behind this standardization.
- Whether these settings are appropriate for each individual service based on their specific load patterns and resource requirements.
- If these changes have been tested in a staging environment to ensure they don't negatively impact service availability or performance.
To help verify the current deployment status and HPA configurations, you can run the following script:
#!/bin/bash # This script assumes you have kubectl configured to access your cluster # and the necessary permissions to view deployments and HPAs # Get the current deployment and HPA status for the affected services services=( "judicial-system-backend" "judicial-system-digital-mailbox-api" "judicial-system-message-handler" "judicial-system-robot-api" "judicial-system-web" "judicial-system-xrd-api" ) for service in "${services[@]}"; do echo "Checking deployment for $service" kubectl get deployment -n judicial-system $service echo "Checking HPA for $service" kubectl get hpa -n judicial-system $service echo "---" doneAlso applies to: 336-338, 430-432, 524-526, 618-620, 712-714
108-108: Confirm the impact of blocking subpoena creationThe new environment variable
BLOCKED_API_INTEGRATION: 'CREATE_SUBPOENA'aligns with the PR objective to block subpoena creation in production. However, ensure that this change doesn't negatively impact any dependent systems or user workflows.To verify the impact, you can run the following script:
✅ Verification successful
Impact of Blocking
CREATE_SUBPOENAConfirmedThe environment variable
BLOCKED_API_INTEGRATION: 'CREATE_SUBPOENA'is defined in both staging and production configurations. Its usage inpolice.config.tsensures that subpoena creation is appropriately blocked without affecting other functionalities.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any code that might be affected by the blocked CREATE_SUBPOENA integration rg -i "CREATE_SUBPOENA" --type ts --type jsLength of output: 359
108-108: Review resource allocation and scaling changesThe following changes have been made to the
judicial-system-backendconfiguration:
- CPU limit increased from 350m to 400m
- Memory limit increased from 512Mi to 1024Mi
nginxRequestsIratein HPA scaling increased from 5 to 8These changes will affect the performance and scaling behavior of the service. Please ensure that:
- The new resource limits are sufficient for the expected load.
- The increased
nginxRequestsIratevalue is appropriate for the service's traffic patterns.- These changes have been tested in a staging environment to verify their impact.
To help verify these changes, you can run the following script to check the current resource usage:
Also applies to: 237-240, 242-245
Datadog ReportAll test runs ✅ 79 Total Test Services: 0 Failed, 77 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (5)
|
unakb
changed the title
What
Block calling the police API for subpoena creation on staging and production.
Why
Because we don't want to send these requests in those environments yet. We want to be able to control when this starts working, espeically on production, but want to be able to test it on dev so it has been deployed.
Also to stop spamming our logs with pointless errors.
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Chores