Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(driving-license): allow 65+ to continue without quality photo #16510

Merged
merged 1 commit into from
Oct 22, 2024

Conversation

stjanilofts
Copy link
Member

@stjanilofts stjanilofts commented Oct 22, 2024

Summary by CodeRabbit

  • New Features
    • Introduced a new environment variable USER_NOTIFICATION_API_URL to enhance communication with the user notification service.
  • Improvements
    • Updated health check paths for various services to improve reliability.
    • Adjusted resource allocations and scaling configurations for better performance and scalability.
    • Refined ingress settings for enhanced service management and routing.
  • Bug Fixes
    • Improved eligibility determination logic for driving license applications.

@stjanilofts stjanilofts requested review from a team as code owners October 22, 2024 13:54
Copy link
Contributor

coderabbitai bot commented Oct 22, 2024

Walkthrough

The pull request introduces significant updates across multiple files, primarily focusing on the integration of the userNotificationService and the addition of the USER_NOTIFICATION_API_URL environment variable. Changes are made to the serviceSetup function to accommodate this new service, alongside extensive modifications to environment variables, resource configurations, health checks, and horizontal pod autoscaler settings in various service YAML files. Additionally, updates to the eligibility logic for driving license applications are included, refining how eligibility is determined for specific application types.

Changes

File Path Change Summary
apps/application-system/api/infra/application-system-api.ts - Added parameter userNotificationService: ServiceBuilder<'services-user-notification'> to serviceSetup function.
- Introduced environment variable USER_NOTIFICATION_API_URL.
charts/islandis/values.dev.yaml - Added USER_NOTIFICATION_API_URL to application-system-api service.
- Updated resource limits for api and services-sessions services.
- Modified health check paths for service-portal and user-notification services.
- Adjusted HPA settings for web and endorsement-system-api.
charts/islandis/values.prod.yaml - Added USER_NOTIFICATION_API_URL to application-system-api service.
- Standardized health check paths.
- Updated HPA settings and resource allocations for several services.
charts/islandis/values.staging.yaml - Added USER_NOTIFICATION_API_URL to application-system-api service.
- Updated health check paths for user-notification and download-service.
- Adjusted resource limits and HPA settings for several services.
infra/src/uber-charts/islandis.ts - Updated instantiation of userNotificationService to include userProfileApi.
- Removed previous implementation of userNotificationService.
libs/application/templates/driving-license/src/fields/EligibilitySummary/useEligibility.ts - Modified eligibility logic for B_FULL_RENEWAL_65 application type, including checks for hasQualityPhoto and hasExtendedDrivingLicense.

Possibly related PRs

  • fix(application-system): Add notification url (#16503) #16507: This PR is directly related as it adds the USER_NOTIFICATION_API_URL environment variable and modifies the serviceSetup function to include the userNotificationService, which aligns with the changes made in the main PR regarding the integration of the user notification service.
  • fix(driving-license): allow continuing without quality photo #16509: This PR modifies the useEligibility function, which is relevant as it also deals with the eligibility logic for driving license applications, specifically for the B_FULL_RENEWAL_65 category, which is mentioned in the main PR's context of service updates.

Suggested labels

automerge

Suggested reviewers

  • kksteini
  • rafnarnason

📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 1b1edbe and d166fb3.

📒 Files selected for processing (6)
  • apps/application-system/api/infra/application-system-api.ts (2 hunks)
  • charts/islandis/values.dev.yaml (1 hunks)
  • charts/islandis/values.prod.yaml (1 hunks)
  • charts/islandis/values.staging.yaml (1 hunks)
  • infra/src/uber-charts/islandis.ts (1 hunks)
  • libs/application/templates/driving-license/src/fields/EligibilitySummary/useEligibility.ts (0 hunks)
💤 Files with no reviewable changes (1)
  • libs/application/templates/driving-license/src/fields/EligibilitySummary/useEligibility.ts
🧰 Additional context used
📓 Path-based instructions (1)
apps/application-system/api/infra/application-system-api.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 (13)
infra/src/uber-charts/islandis.ts (2)

72-74: Improved integration of userNotificationService

The changes enhance the integration between the user notification service and the user profile API. This modification aligns well with the overall system architecture and should improve the functionality of user notifications.


81-81: Verify integration of userNotificationService in appSystemApi

The addition of userNotificationService to the appSystemApi setup is a positive change that should enable user notification functionalities within the application system. This modification is consistent with the earlier changes.

To ensure proper integration, please run the following script to verify the usage of userNotificationService in the application system API:

✅ Verification successful

Integration of userNotificationService in appSystemApi Verified Successfully

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of userNotificationService in the application system API

# Test: Search for userNotificationService usage in application system API files
rg -p 'userNotificationService' apps/application-system/api

Length of output: 267

apps/application-system/api/infra/application-system-api.ts (2)

125-125: LGTM: New userNotificationService parameter added

The addition of the userNotificationService parameter to the serviceSetup function is consistent with the existing pattern for other services. This change improves the modularity of the code and aligns with NextJS best practices for API routes.


260-262: LGTM: USER_NOTIFICATION_API_URL environment variable added

The new USER_NOTIFICATION_API_URL environment variable is correctly set up using the ref function, consistent with other service URL configurations. This addition enhances the service setup by integrating the user notification service.

charts/islandis/values.staging.yaml (6)

Line range hint 1-1600: Overall changes in the staging environment configuration are well-coordinated and appropriate.

The updates to charts/islandis/values.staging.yaml reflect a comprehensive effort to enhance the staging environment:

  1. Integration of the user notification service
  2. Adjustments to resource limits and requests for various services
  3. Improvements to scalability through HPA setting updates
  4. Optimization of proxy behavior through ingress annotation changes

These changes should collectively improve the performance, scalability, and resource utilization of the system. However, it's crucial to monitor the effects of these changes closely after deployment to ensure they meet the intended goals without introducing any unforeseen issues.

After deploying these changes, implement a comprehensive monitoring plan that includes:

  1. Resource usage tracking for CPU and memory across all updated services
  2. Scaling behavior observation, especially for services with adjusted HPA settings
  3. Performance metrics collection, including response times and error rates
  4. Proxy and ingress controller behavior monitoring

Use a combination of Kubernetes native tools (kubectl, top) and any external monitoring solutions you have in place to gather this data. Regular review of these metrics will help validate the effectiveness of these changes and identify any areas that may need further adjustment.


Line range hint 1-1600: Maximum replica count increase for the services-sessions service is appropriate.

The maximum replica count for the services-sessions service has been increased to 10, which aligns with the summary mentioning adjustments to HPA (Horizontal Pod Autoscaler) settings. This change should improve the service's ability to handle higher loads.

To ensure this change is effective and the service scales appropriately, monitor the scaling behavior after deployment. You can use the following commands to check the HPA status and the number of running pods:

# Check HPA status
kubectl get hpa -n services-sessions

# Check the number of running pods
kubectl get pods -n services-sessions -l app=services-sessions

Run these commands periodically, especially during high-load periods, to observe the scaling behavior. If the number of replicas rarely approaches the new maximum, consider adjusting the HPA settings for more efficient resource use.


Line range hint 1-1600: Ingress annotation updates for the web service are appropriate.

The ingress annotations for the web service have been updated to improve proxy buffering settings. This change aligns with the summary mentioning updates to ingress configurations and should optimize proxy behavior.

To ensure these changes have a positive impact on the service's performance, monitor the proxy's behavior after deployment. You can use the following command to check the nginx ingress controller logs for any buffering-related issues:

kubectl logs -n nginx-ingress -l app=nginx-ingress-controller | grep -i buffer

Additionally, monitor the overall performance of the web service, paying attention to response times and any potential errors that might be related to proxy buffering. You may want to use application performance monitoring tools to get more detailed insights.


617-617: New environment variable for user notification service integration looks good.

The addition of USER_NOTIFICATION_API_URL is consistent with the integration of the user notification service mentioned in the summary. The URL format is correct for internal Kubernetes service discovery.

Please ensure that the user notification service is properly set up and accessible at the specified URL. You can verify this by running the following command in the cluster:

This will attempt to reach the user notification service. Check the output to ensure the service is responding correctly.


Line range hint 1-1600: Memory request increase for the services-university-gateway service is appropriate.

The memory request for the services-university-gateway service has been increased from 256Mi to 384Mi, which is in line with the summary mentioning updates to resource requests. This change should provide more RAM for the service.

To ensure this change is effective and not excessive, please monitor the memory usage of the services-university-gateway service after deployment. You can use the following command to check the memory usage:

Run this command periodically after deployment to observe the memory usage patterns. If the usage consistently remains well below the new request, consider adjusting it to optimize resource allocation.


Line range hint 1-1600: CPU limit increase for the api service is appropriate.

The CPU limit for the api service has been increased from 1200m to 1600m, which aligns with the summary mentioning updates to resource limits. This change should provide more computational resources for the service.

To ensure this change is effective and not excessive, please monitor the CPU usage of the api service after deployment. You can use the following command to check the CPU usage:

Run this command periodically after deployment to observe the CPU usage patterns. If the usage consistently remains well below the new limit, consider adjusting it to optimize resource allocation.

charts/islandis/values.prod.yaml (1)

Line range hint 1-3686: Reminder: Ensure thorough testing for configuration changes.

While only one change is visible in this diff, it's crucial to remember that this configuration file (values.prod.yaml) is critical for the entire production environment. Any modifications, no matter how small, can potentially impact multiple services and the overall system stability.

To maintain system integrity, please ensure:

  1. All changes are thoroughly tested in a staging environment that mirrors production.
  2. A rollback plan is in place before applying changes to production.
  3. Monitoring is set up to quickly detect any issues post-deployment.

Consider running a diff between this file and the staging configuration to verify consistency:

charts/islandis/values.dev.yaml (2)

Line range hint 1-2395: Summary of changes and recommendations

The main change in this configuration file is the addition of the USER_NOTIFICATION_API_URL to the application-system-api service. This is a positive change that enables communication with the user notification service. The user-notification service itself is properly configured with necessary environment variables and secrets.

However, to improve consistency and functionality across the system, consider the following recommendations:

  1. Add the USER_NOTIFICATION_API_URL to other relevant services that might need to interact with the user notification service, such as service-portal-api.
  2. Review all services to identify any that might benefit from having access to the user notification service and add the environment variable accordingly.
  3. Ensure that the SERVICE_PORTAL_CLICK_ACTION_URL in the user-notification service is correct for the development environment.

These changes will help ensure that all necessary services can communicate with the user notification service and maintain consistency across the system.


Line range hint 1957-2122: Verify user-notification service configuration

The user-notification service is properly configured with necessary environment variables and secrets. However, there are a couple of points to consider:

  1. The USER_NOTIFICATION_API_URL is not present in the user-notification service itself. This is expected as the service doesn't need to call itself.

  2. The SERVICE_PORTAL_CLICK_ACTION_URL is set to 'https://island.is/minarsidur'. Ensure this URL is correct for the development environment.

Let's verify if the SERVICE_PORTAL_CLICK_ACTION_URL is consistent across environments:

✅ Verification successful

SERVICE_PORTAL_CLICK_ACTION_URL is consistent across all environments

The SERVICE_PORTAL_CLICK_ACTION_URL is set to 'https://island.is/minarsidur' in development, production, and staging environments. Please confirm that using the same URL across all environments is intentional.

  • charts/islandis/values.dev.yaml
  • charts/islandis/values.prod.yaml
  • charts/islandis/values.staging.yaml
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check SERVICE_PORTAL_CLICK_ACTION_URL across different environment files
grep -n "SERVICE_PORTAL_CLICK_ACTION_URL" charts/islandis/values.*.yaml

Length of output: 782


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 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.

@stjanilofts stjanilofts changed the base branch from main to pre-release/32.2.0 October 22, 2024 13:55
@stjanilofts stjanilofts added the deprecated:automerge (Disabled) Merge this PR as soon as all checks pass label Oct 22, 2024
Copy link

codecov bot commented Oct 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (pre-release/32.2.0@78ce457). Learn more about missing BASE report.

Additional details and impacted files

Impacted file tree graph

@@                  Coverage Diff                  @@
##             pre-release/32.2.0   #16510   +/-   ##
=====================================================
  Coverage                      ?   36.77%           
=====================================================
  Files                         ?     6847           
  Lines                         ?   141870           
  Branches                      ?    40421           
=====================================================
  Hits                          ?    52167           
  Misses                        ?    89703           
  Partials                      ?        0           
Flag Coverage Δ
api 3.37% <ø> (?)
application-system-api 41.34% <ø> (?)
application-template-api-modules 27.78% <ø> (?)
application-templates-driving-license 18.34% <ø> (?)
application-ui-shell 21.36% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...se/src/fields/EligibilitySummary/useEligibility.ts 0.00% <ø> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78ce457...d166fb3. Read the comment docs.

@kodiakhq kodiakhq bot merged commit 1b70f1c into pre-release/32.2.0 Oct 22, 2024
51 of 61 checks passed
@kodiakhq kodiakhq bot deleted the fix/dl-65plus-photo branch October 22, 2024 14:02
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.

Caution

Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (1)
charts/islandis/values.dev.yaml (1)

Line range hint 1957-2122: Consider adding USER_NOTIFICATION_API_URL to other relevant services

While the USER_NOTIFICATION_API_URL has been added to the application-system-api service, it's not present in other services that might need to interact with the user notification service. Consider adding this environment variable to other relevant services, such as service-portal-api.

Add the following environment variable to the service-portal-api service configuration:

USER_NOTIFICATION_API_URL: 'http://web-user-notification.user-notification.svc.cluster.local'

Also, review other services that might need to interact with the user notification service and add this environment variable as necessary.

🛑 Comments failed to post (2)
charts/islandis/values.prod.yaml (1)

608-608: 💡 Codebase verification

Inconsistent USER_NOTIFICATION_API_URL across services and environments.

The USER_NOTIFICATION_API_URL is configured differently in various services and environments, which might lead to inconsistent behavior.

  • ./charts/identity-server/values.prod.yaml uses https://user-notification.internal.island.is
  • Other environment files in charts/islandis use http://web-user-notification.user-notification.svc.cluster.local
🔗 Analysis chain

New environment variable added for user notification service.

A new environment variable USER_NOTIFICATION_API_URL has been added to the application-system-api service. This suggests integration with a user notification service, which is a good practice for improving user experience and communication.

To ensure this change is consistent across the system, let's check if other services that might need user notifications have similar configurations:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for USER_NOTIFICATION_API_URL in other services
grep -R "USER_NOTIFICATION_API_URL" . | grep -v "application-system-api:"

Length of output: 1761

charts/islandis/values.dev.yaml (1)

620-620: 💡 Codebase verification

Multiple services are missing the USER_NOTIFICATION_API_URL environment variable.

  • Service air-discount-scheme-api is missing USER_NOTIFICATION_API_URL.
  • Service air-discount-scheme-backend is missing USER_NOTIFICATION_API_URL.
  • Service air-discount-scheme-web is missing USER_NOTIFICATION_API_URL.
  • Service api is missing USER_NOTIFICATION_API_URL.
  • Service application-system-api is missing USER_NOTIFICATION_API_URL.
  • Service application-system-api-worker is missing USER_NOTIFICATION_API_URL.
  • Service application-system-form is missing USER_NOTIFICATION_API_URL.
  • Service consultation-portal is missing USER_NOTIFICATION_API_URL.
  • Service contentful-apps is missing USER_NOTIFICATION_API_URL.
  • Service contentful-entry-tagger-service is missing USER_NOTIFICATION_API_URL.
  • Service download-service is missing USER_NOTIFICATION_API_URL.
  • Service endorsement-system-api is missing USER_NOTIFICATION_API_URL.
  • Service external-contracts-tests is missing USER_NOTIFICATION_API_URL.
  • Service github-actions-cache is missing USER_NOTIFICATION_API_URL.
  • Service global is missing USER_NOTIFICATION_API_URL.
  • Service icelandic-names-registry-backend is missing USER_NOTIFICATION_API_URL.
  • Service island-ui-storybook is missing USER_NOTIFICATION_API_URL.
  • Service license-api is missing USER_NOTIFICATION_API_URL.
  • Service namespaces is missing USER_NOTIFICATION_API_URL.
  • Service portals-admin is missing USER_NOTIFICATION_API_URL.
  • Service regulations-admin-backend is missing USER_NOTIFICATION_API_URL.
  • Service search-indexer-service is missing USER_NOTIFICATION_API_URL.
  • Service service-portal is missing USER_NOTIFICATION_API_URL.
  • Service service-portal-api is missing USER_NOTIFICATION_API_URL.
  • Service services-documents is missing USER_NOTIFICATION_API_URL.
  • Service services-sessions is missing USER_NOTIFICATION_API_URL.
  • Service services-sessions-cleanup is missing USER_NOTIFICATION_API_URL.
  • Service services-sessions-worker is missing USER_NOTIFICATION_API_URL.
  • Service services-university-gateway is missing USER_NOTIFICATION_API_URL.
  • Service services-university-gateway-worker is missing USER_NOTIFICATION_API_URL.
  • Service skilavottord-web is missing USER_NOTIFICATION_API_URL.
  • Service skilavottord-ws is missing USER_NOTIFICATION_API_URL.
  • Service user-notification is missing USER_NOTIFICATION_API_URL.
  • Service user-notification-cleanup-worker is missing USER_NOTIFICATION_API_URL.
  • Service user-notification-worker is missing USER_NOTIFICATION_API_URL.
  • Service web is missing USER_NOTIFICATION_API_URL.
  • Service xroad-collector is missing USER_NOTIFICATION_API_URL.
🔗 Analysis chain

New environment variable added for user notification service.

A new environment variable USER_NOTIFICATION_API_URL has been added to the application-system-api service. This is a good addition as it allows the service to communicate with the user notification service.

Let's verify if other services that might need user notifications have this environment variable:


[/run_scripts]

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if other services have the USER_NOTIFICATION_API_URL
grep -n "USER_NOTIFICATION_API_URL" charts/islandis/values.dev.yaml

Length of output: 271


Script:

#!/bin/bash
# List all services in the YAML file
services=$(grep -E '^[a-zA-Z0-9_-]+:' charts/islandis/values.dev.yaml | awk -F: '{print $1}')

# Check each service for USER_NOTIFICATION_API_URL
for service in $services; do
  if ! grep -q "USER_NOTIFICATION_API_URL" charts/islandis/values.dev.yaml | grep "^$service:"; then
    echo "Service '$service' is missing USER_NOTIFICATION_API_URL."
  fi
done

Length of output: 11211

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecated:automerge (Disabled) Merge this PR as soon as all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants