fix(ids-api): Zendesk config #16820
fix(ids-api): Zendesk config #16820
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces new environment variables and secrets related to Zendesk integration across multiple configuration files for the identity server. Specifically, it adds Changes
Possibly related PRs
Suggested reviewers
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
|
GunnlaugurG
added
the
deprecated:automerge
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
charts/identity-server/values.prod.yaml (1)
Line range hint
438-526: Consider adjusting CPU utilization target for better scaling responsiveness.While increasing max replicas to 15 is good for handling higher load, the CPU utilization target of 90% might be too high for responsive autoscaling. A high target means the HPA will wait until pods are under significant load before scaling up, which could impact service performance during traffic spikes.
Consider lowering the
cpuAverageUtilizationtarget to 70-80% for more proactive scaling:hpa: scaling: metric: - cpuAverageUtilization: 90 + cpuAverageUtilization: 75 nginxRequestsIrate: 5charts/identity-server/values.dev.yaml (1)
441-441: Consider using a shared configuration for Zendesk settings.The Zendesk configuration is currently duplicated across multiple services. Consider moving these common configurations to a shared configuration section to improve maintainability and reduce the risk of inconsistencies.
Example approach using global variables:
global: env: + ZENDESK_CONTACT_FORM_SUBDOMAIN: 'digitaliceland' secrets: + ZENDESK_CONTACT_FORM_EMAIL: '/k8s/api/ZENDESK_CONTACT_FORM_EMAIL' + ZENDESK_CONTACT_FORM_TOKEN: '/k8s/api/ZENDESK_CONTACT_FORM_TOKEN' + ZENDESK_WEBHOOK_SECRET_GENERAL_MANDATE: '/k8s/services-auth/ZENDESK_WEBHOOK_SECRET_GENERAL_MANDATE'Then reference these global values in each service.
Also applies to: 527-529
charts/identity-server/values.staging.yaml (1)
441-441: Consider documenting Zendesk integrationSince this is a new integration being added, consider adding documentation about:
- Purpose of the Zendesk integration
- Required configuration steps
- Contact form workflow
Also applies to: 527-529
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
apps/services/auth/ids-api/infra/ids-api.ts(1 hunks)charts/identity-server/values.dev.yaml(2 hunks)charts/identity-server/values.prod.yaml(2 hunks)charts/identity-server/values.staging.yaml(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/services/auth/ids-api/infra/ids-api.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
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 (8)
apps/services/auth/ids-api/infra/ids-api.ts (2)
92-96: LGTM: Environment variable configuration is consistent.
The ZENDESK_CONTACT_FORM_SUBDOMAIN configuration follows the established pattern and maintains consistency across all environments.
Line range hint 1-150: Consider adding Zendesk-specific health checks.
The service configuration includes proper health checks, but with the addition of Zendesk integration, consider:
- Adding specific health checks for the Zendesk connectivity in your
/health/checkendpoint - Documenting the expected behavior when Zendesk services are unavailable
#!/bin/bash
# Description: Look for health check implementations in the codebase
# Search for health check implementations
echo "Searching for health check implementations:"
rg -A 5 "health.*check" --type typescript
# Search for Zendesk-related health monitoring
echo "Searching for Zendesk-related health monitoring:"
rg -A 5 "zendesk.*health|health.*zendesk" --type typescriptcharts/identity-server/values.prod.yaml (2)
438-438: LGTM: Zendesk configuration is properly implemented.
The Zendesk configuration is consistently applied across services with proper secret management through AWS Parameter Store.
Also applies to: 524-526
Line range hint 438-526: LGTM: InitContainer configuration follows best practices.
The database migration and seeding init containers are well-configured with:
- Appropriate resource limits and requests
- Proper environment variables and secrets
- Correct sequencing (migrations before seeding)
charts/identity-server/values.dev.yaml (1)
Line range hint 441-529: Resource configuration and scaling parameters look appropriate.
The configuration includes:
- Well-defined resource limits and requests for both main containers and init containers
- Appropriate scaling parameters with max replicas set to 15 for services-auth-ids-api
- Proper health check configurations
charts/identity-server/values.staging.yaml (3)
441-441: LGTM: Zendesk configuration added correctly
The Zendesk configuration has been properly added to the services-auth-ids-api service with:
- Environment variable for the subdomain
- Required secrets for email, token and webhook
Also applies to: 527-529
Line range hint 441-529: Resource configuration looks appropriate
The resource configuration for services-auth-ids-api is well-balanced:
- Max replicas increased to 15 provides good scalability
- CPU and memory limits (800m/768Mi) are reasonable for the service's requirements
441-441: Verify Zendesk secret paths across environments
Let's ensure the Zendesk secret paths are consistent across environments.
Also applies to: 527-529
✅ Verification successful
Zendesk secret paths are consistent across environments.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if Zendesk secret paths are consistent across environments
# Expected: Same secret paths should be used in dev and prod environments
echo "Checking Zendesk secret paths in values files..."
rg -A 1 "ZENDESK_.*TOKEN" "charts/identity-server/values*.yaml"
Length of output: 248
Script:
#!/bin/bash
# Description: List all values*.yaml files in charts/identity-server to verify their existence
echo "Listing all values*.yaml files in charts/identity-server..."
fd "values.*\.yaml$" "charts/identity-server"
Length of output: 296
Script:
#!/bin/bash
# Description: Display the contents of values.staging.yaml to verify Zendesk secret paths
echo "Displaying contents of values.staging.yaml..."
cat charts/identity-server/values.staging.yaml
Length of output: 31814
Script:
#!/bin/bash
# Description: Search for Zendesk secret paths across all environments
echo "Searching for Zendesk secret paths in all values*.yaml files..."
rg "ZENDESK_.*TOKEN" "charts/identity-server/values*.yaml"
Length of output: 273
Script:
#!/bin/bash
# Description: Search for all Zendesk secret paths across all environments in values*.yaml files
echo "Searching for all Zendesk secret paths in values*.yaml files..."
rg "ZENDESK_.*" charts/identity-server/values*.yaml
Length of output: 7171
Script:
#!/bin/bash
# Description: Use fd to find all values*.yaml files and search for Zendesk secret paths
echo "Using fd to locate values*.yaml files and searching for Zendesk secret paths..."
fd "values.*\.yaml$" charts/identity-server | xargs rg "ZENDESK_.*"
Length of output: 7133
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
apps/services/auth/ids-api/infra/ids-api.ts (1)
92-102: Add documentation for Zendesk integrationConsider adding comments to document:
- The purpose of these Zendesk configurations
- Required Zendesk setup steps
- Any dependencies on other services
Add documentation above the Zendesk configurations:
+ // Zendesk integration configuration for contact form and webhook functionality + // Required setup: + // 1. Zendesk account with API access + // 2. Contact form configuration + // 3. Webhook configuration for general mandate notifications ZENDESK_CONTACT_FORM_SUBDOMAIN: {charts/identity-server/values.prod.yaml (1)
438-438: Consider moving the Zendesk subdomain to a configurable parameterThe Zendesk subdomain is hardcoded as 'digitaliceland'. To improve maintainability and flexibility across environments, consider moving this to a configurable parameter in the parameter store, similar to how other Zendesk configurations are handled.
- ZENDESK_CONTACT_FORM_SUBDOMAIN: 'digitaliceland' + ZENDESK_CONTACT_FORM_SUBDOMAIN: '/k8s/api/ZENDESK_CONTACT_FORM_SUBDOMAIN'charts/identity-server/values.dev.yaml (1)
Line range hint
441-529: Resource and scaling configurations look appropriate.The configuration provides adequate resources for the service:
- CPU limit of 800m and memory limit of 768Mi should be sufficient for the service
- HPA configuration with max 15 replicas ensures good scalability
- These resource allocations should help resolve the startup issues mentioned in the PR objectives
Consider monitoring the actual resource usage in development environment to fine-tune these limits if needed.
charts/identity-server/values.staging.yaml (1)
Line range hint
441-529: Consider optimizing HPA settingsWhile increasing max replicas to 15 provides better scalability, the CPU utilization threshold of 90% might be too high:
- High CPU threshold could lead to delayed scaling
- Consider lowering to 70-80% for more proactive scaling
scaling: metric: - cpuAverageUtilization: 90 + cpuAverageUtilization: 75
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
apps/services/auth/ids-api/infra/ids-api.ts(1 hunks)charts/identity-server/values.dev.yaml(2 hunks)charts/identity-server/values.prod.yaml(2 hunks)charts/identity-server/values.staging.yaml(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/services/auth/ids-api/infra/ids-api.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
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 (7)
apps/services/auth/ids-api/infra/ids-api.ts (1)
92-96: Verify the Zendesk subdomain configuration across environments
The same subdomain 'digitaliceland' is used across all environments (dev, staging, prod). While this might be intentional, typically different subdomains are used for development and staging environments to isolate testing from production.
charts/identity-server/values.prod.yaml (2)
524-526: LGTM: Zendesk secrets properly configured
The Zendesk secrets are correctly configured using parameter store references, maintaining security best practices.
438-438: Verify Zendesk configuration in parameter store
Let's ensure all required Zendesk parameters are available in the parameter store.
Also applies to: 524-526
charts/identity-server/values.dev.yaml (1)
441-441: LGTM! Zendesk configuration is properly structured.
The Zendesk configuration is consistently implemented across services with proper secret management.
Let's verify the existence of the required secrets:
Also applies to: 527-529
charts/identity-server/values.staging.yaml (3)
441-441: LGTM: Zendesk configuration properly added
The Zendesk configuration is correctly implemented with appropriate secret management for sensitive data.
Also applies to: 527-529
Line range hint 441-529: LGTM: Init containers properly configured
The init containers for database migration and seeding are well-configured with:
- Appropriate resource limits and requests
- Clear separation of migration and seeding tasks
- Proper database extension configuration
Line range hint 441-529: LGTM: Resource allocation is well-balanced
The resource allocation is appropriate with:
- Sufficient memory headroom for the application
- Reasonable CPU limits allowing for burst capacity
- Well-defined resource constraints for init containers
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16820 +/- ##
==========================================
- Coverage 36.44% 36.44% -0.01%
==========================================
Files 6852 6852
Lines 143527 143508 -19
Branches 40961 40950 -11
==========================================
- Hits 52303 52295 -8
+ Misses 91224 91213 -11
Flags with carried forward coverage won't be shown. Click here to find out more. see 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 5 Total Test Services: 0 Failed, 5 Passed Test Services
|
oddsson
removed
the
deprecated:automerge
GunnlaugurG
added
the
deprecated:automerge
* fix zendesk config for auth-ids-api * chore: charts update dirty files * chore: charts update dirty files --------- Co-authored-by: andes-it <[email protected]> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
What
Added Zendesk config to ids-api so it will start
Why
because ids-api is failing to start on dev
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
initContainerfor database migration and seeding tasks in the identity server.services-auth-ids-apito 15 for better scalability.