fix: allow empty envs in helm values #17056
Conversation
WalkthroughThe pull request introduces multiple modifications across various YAML configuration files for different services. Key changes include the addition of new environment variables, updates to health check paths, and adjustments to Horizontal Pod Autoscaler (HPA) settings. The configurations for the Changes
Possibly related PRs
Suggested labels
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
|
![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 (10)
charts/services/application-system-form/values.dev.yaml (1)
Line range hint
1-5: Note about generated fileThis is an automatically generated file. Any changes should be made through the source configuration and regenerated using
yarn charts.charts/services/judicial-system-api/values.dev.yaml (1)
33-33: Consider documenting the purpose of HIDDEN_FEATURES.While the empty value is valid, it would be helpful to document the purpose and usage of this environment variable for future reference.
Consider adding a comment in the chart's README or documentation explaining:
- The purpose of HIDDEN_FEATURES
- Expected values (including empty string)
- Impact on the application's behavior
charts/services/judicial-system-api/values.staging.yaml (1)
⚠️ Changes must be made through the chart generation processThe verification confirms that:
- HIDDEN_FEATURES is consistently added with an empty value ('') across all environments (dev, staging, and prod)
- The files are generated through a TypeScript-based chart generation system (
generate-chart-values.ts)- The warning header is automatically added by the generation process
Please make your changes through the chart generation system by:
- Running
yarn chartsin the infra directory- If HIDDEN_FEATURES needs to be added, modify the chart generation source code instead of editing the values files directly
- The direct modification of
values.staging.yamlbypasses the generation system and will be overwritten on the next chart generation.🔗 Analysis chain
Line range hint
1-7:⚠️ Caution: This file is auto-generatedThe file header explicitly states not to edit this file manually and instructs to run "yarn charts" instead. Please ensure you're following the correct process to modify this configuration.
Let's verify the generation process and consistency across environments:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for the presence and configuration of HIDDEN_FEATURES across environments # Expected: Similar configuration in dev and prod environments # Check for HIDDEN_FEATURES in dev and prod values echo "Checking dev environment..." fd -t f "values.dev.yaml" charts/services/judicial-system-api -x grep -l "HIDDEN_FEATURES" echo "Checking prod environment..." fd -t f "values.prod.yaml" charts/services/judicial-system-api -x grep -l "HIDDEN_FEATURES" # Check for the chart generation script echo "Checking for chart generation script..." fd -t f "package.json" . -x grep -A 5 "\"charts\":"Length of output: 1134
Script:
#!/bin/bash # Let's check the actual content of the HIDDEN_FEATURES in all environments echo "Dev environment HIDDEN_FEATURES:" rg "HIDDEN_FEATURES" charts/services/judicial-system-api/values.dev.yaml -A 1 echo -e "\nStaging environment HIDDEN_FEATURES:" rg "HIDDEN_FEATURES" charts/services/judicial-system-api/values.staging.yaml -A 1 echo -e "\nProd environment HIDDEN_FEATURES:" rg "HIDDEN_FEATURES" charts/services/judicial-system-api/values.prod.yaml -A 1 # Let's also check the chart generation source echo -e "\nChecking chart generation source:" fd -t f "generate-chart-values.ts" . -x cat {}Length of output: 3398
infra/src/dsl/output-generators/map-to-helm-values.ts (2)
78-78: LGTM! Consider using optional chaining for cleaner code.The conditional health check port assignment is a good improvement. However, the code could be more concise.
Consider using optional chaining:
- if (serviceDef.healthPort) { - result.healthCheck.port = serviceDef.healthPort - } + result.healthCheck.port = serviceDef.healthPort ?? undefined;Also applies to: 95-98
432-434: LGTM! Consider using object spread for cleaner code.The conditional image assignment is correct, but the code could be more concise.
Consider using object spread:
- if (c.image) { - result.image = c.image - } + result = { + ...result, + ...(c.image && { image: c.image }), + };charts/judicial-system/values.dev.yaml (2)
Line range hint
42-50: Consider separating liveness and readiness probe pathsMost services use
/livenessfor both liveness and readiness probes, except forjudicial-system-web. Consider implementing separate endpoints for readiness probes across all services to better reflect their distinct purposes:
- Liveness: Check if the service is running
- Readiness: Verify if the service is ready to handle traffic
healthCheck: liveness: initialDelaySeconds: 3 path: '/liveness' timeoutSeconds: 3 readiness: initialDelaySeconds: 3 - path: '/liveness' + path: '/readiness' timeoutSeconds: 3Also applies to: 447-455
Line range hint
51-57: Review HPA CPU utilization targetThe CPU utilization target of 90% across all services might be too aggressive:
- High target leaves little room for traffic spikes
- Could lead to delayed scaling during sudden load increases
Consider lowering the CPU utilization target:
hpa: scaling: metric: - cpuAverageUtilization: 90 + cpuAverageUtilization: 70 nginxRequestsIrate: 8 replicas: max: 10charts/judicial-system/values.staging.yaml (1)
Health check path patterns need standardization across services
Based on the verification results, there are indeed inconsistent patterns in health check paths across services:
- Most common patterns found:
/livenessfor both liveness and readiness (e.g., judicial-system-api, skilavottord-web)/livenessfor liveness and/readinessfor readiness (e.g., web, services-documents)/livenessfor liveness and/health/checkfor readiness (e.g., user-notification, services-auth-public-api)/for both checks (e.g., user-notification-cleanup-worker, xroad-collector)
- Some services use custom prefixed paths:
/stjornbord/bff/livenessand/stjornbord/bff/health/check(services-bff-portals-admin)/backend/livenessand/backend/health/check(services-auth-admin-api)Recommend standardizing to:
/livenessfor liveness probes/readinessfor readiness probesThis will improve maintainability and make monitoring more consistent.
🔗 Analysis chain
Line range hint
42-50: Consider standardizing health check paths across servicesThere's an inconsistency in health check paths across services. For example:
- judicial-system-api uses
/livenessfor both liveness and readiness- judicial-system-web uses different paths:
/livenessfor liveness and/readinessfor readinessConsider standardizing this pattern across all services for consistency and maintainability.
Also applies to: 447-455
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check health check path consistency across services # Look for health check configurations rg "path: '/[^']*'" -A 1 | grep -E "path:|initialDelaySeconds"Length of output: 48264
charts/judicial-system/values.prod.yaml (1)
Health check paths should be explicitly defined
The codebase shows that the standard health check implementation in
libs/infra-express-serveruses the/livenesspath. The root path ('/') should not be used for health checks as:
- The infrastructure code explicitly defines
/livenessas the health check endpoint- All other services in the cluster consistently use
/livenessand/readinesspaths- Using root path could interfere with other route handlers and make debugging more difficult
Recommendations:
- Update the health check paths in
judicial-system-schedulerconfiguration to use/livenessand/readinessfor consistency- Remove the root path configuration and align with the standard health check implementation
🔗 Analysis chain
Line range hint
379-386: Verify root path health check implementationThe health check paths for
judicial-system-schedulerhave been changed to use the root path ('/'). This differs from other services that use specific paths like '/liveness' and '/readiness'. Please verify:
- The root path properly implements both liveness and readiness checks
- Consider maintaining consistency with other services by using specific paths
Let me search for the scheduler's health check implementation to verify if it properly handles both liveness and readiness checks at the root path.
Let me check the scheduler's implementation and routes to understand how health checks are handled.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for health check implementations echo "Searching for health check implementations in scheduler..." ast-grep --pattern 'app.get("/", $$$)' echo "Comparing with other services' health check paths..." rg -A 2 "path: '/liveness|path: '/readiness"Length of output: 84041
Script:
#!/bin/bash # Search for scheduler's source code and health check implementation echo "Looking for scheduler source files..." fd scheduler echo -e "\nSearching for health check implementation in scheduler..." rg -A 5 "app.(get|use)\('/',"Length of output: 985
Script:
#!/bin/bash # Check scheduler's implementation echo "Checking scheduler routes and health checks..." rg -A 10 "router|app.get|healthcheck" apps/judicial-system/scheduler/ echo -e "\nChecking express server health check implementations..." rg -A 10 "router|app.get|healthcheck" libs/infra-express-server/Length of output: 2033
charts/services/api/values.staging.yaml (1)
Line range hint
1-450: Consider using external secrets management for sensitive valuesThe configuration file contains numerous sensitive values referenced via paths (e.g., API keys, client secrets, passwords). While these are properly externalized using secret references, consider using a more robust external secrets management solution like AWS Secrets Manager or HashiCorp Vault for enhanced security.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (20)
charts/islandis/values.dev.yaml(1 hunks)charts/islandis/values.prod.yaml(2 hunks)charts/islandis/values.staging.yaml(2 hunks)charts/judicial-system/values.dev.yaml(2 hunks)charts/judicial-system/values.prod.yaml(2 hunks)charts/judicial-system/values.staging.yaml(1 hunks)charts/services/api/values.prod.yaml(1 hunks)charts/services/api/values.staging.yaml(1 hunks)charts/services/application-system-form/values.dev.yaml(1 hunks)charts/services/application-system-form/values.prod.yaml(1 hunks)charts/services/application-system-form/values.staging.yaml(1 hunks)charts/services/judicial-system-api/values.dev.yaml(1 hunks)charts/services/judicial-system-api/values.prod.yaml(1 hunks)charts/services/judicial-system-api/values.staging.yaml(1 hunks)charts/services/judicial-system-backend/values.dev.yaml(1 hunks)charts/services/judicial-system-backend/values.prod.yaml(1 hunks)infra/src/cli/generate-chart-values.ts(1 hunks)infra/src/dsl/output-generators/map-to-helm-values.ts(4 hunks)infra/src/dsl/service-to-environment/pre-process-service.ts(1 hunks)infra/src/dsl/types/input-types.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- infra/src/dsl/types/input-types.ts
🧰 Additional context used
📓 Path-based instructions (2)
infra/src/dsl/output-generators/map-to-helm-values.ts (1)
Pattern infra/src/dsl/**/*: "Confirm that the code adheres to the following:
- The clarity and expressiveness of the DSL syntax.
- Integration with Helm charts and Kubernetes resources.
- Documentation on how to use the DSL to create complex Helm values."
infra/src/dsl/service-to-environment/pre-process-service.ts (1)
Pattern infra/src/dsl/**/*: "Confirm that the code adheres to the following:
- The clarity and expressiveness of the DSL syntax.
- Integration with Helm charts and Kubernetes resources.
- Documentation on how to use the DSL to create complex Helm values."
🔇 Additional comments (40)
charts/services/application-system-form/values.dev.yaml (1)
27-27: Verify the intentional empty GraphQL path configuration
The empty value for SI_PUBLIC_GRAPHQL_PATH aligns with the PR objective to allow empty environment variables. However, we should verify:
- If this is intentionally left empty for the dev environment
- How this affects the GraphQL endpoint configuration
✅ Verification successful
Empty GraphQL path is the intended configuration
The empty value for SI_PUBLIC_GRAPHQL_PATH is correct and intentionally configured this way. Here's why:
- In the infrastructure code (
application-system-form.ts), the empty value is explicitly set for dev, staging, and prod environments - In
environment.ts, whenSI_PUBLIC_GRAPHQL_PATHis empty, it falls back towindow.location.originas the baseApiUrl - This configuration is consistent across all environments (dev, staging, prod) in both the application-system-form and islandis charts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if this empty configuration is consistent across other environments
# and if there's any documentation explaining this configuration
# Check other environment files for this variable
echo "Checking other environment files for SI_PUBLIC_GRAPHQL_PATH:"
rg "SI_PUBLIC_GRAPHQL_PATH" charts/services/application-system-form/
# Check for any documentation about this configuration
echo -e "\nChecking for related documentation:"
rg -i "graphql.?path" docs/ README.md
Length of output: 632
Script:
#!/bin/bash
# Let's check the application code to understand if this empty path is expected
# and how it's being used
# Search for any usage of this environment variable in the codebase
echo "Checking for SI_PUBLIC_GRAPHQL_PATH usage:"
rg "SI_PUBLIC_GRAPHQL_PATH"
# Check for any configuration or setup files that might explain this
echo -e "\nChecking for related configuration files:"
fd -e yaml -e json -e js -e ts -x rg -l "graphql.*path"
# Look for GraphQL setup or configuration files
echo -e "\nLooking for GraphQL configuration files:"
fd -e ts -e js -x rg -l "graphql.*config"
Length of output: 994
Script:
#!/bin/bash
# Let's check how the environment variable is used in the application code
# Check the environment setup file
echo "Checking environment.ts implementation:"
cat "apps/application-system/form/src/environments/environment.ts"
# Check the infrastructure configuration
echo -e "\nChecking infrastructure configuration:"
cat "apps/application-system/form/infra/application-system-form.ts"
# Check if there's any relevant configuration in next.config.js
echo -e "\nChecking next.config.js:"
cat "apps/web/next.config.js"
Length of output: 9335
charts/services/application-system-form/values.staging.yaml (3)
27-27: LGTM! The empty environment variable aligns with PR objectives.
The addition of SI_PUBLIC_GRAPHQL_PATH: '' is consistent with the PR's goal to allow empty environment variables in Helm values.
Let's verify if this pattern is consistent across other environments:
✅ Verification successful
Empty GraphQL path configuration is consistently defined across all environments
The verification confirms that SI_PUBLIC_GRAPHQL_PATH is set to an empty string ('') consistently across:
- All environments (dev/staging/prod)
- Both service configurations (application-system-form and islandis)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if SI_PUBLIC_GRAPHQL_PATH is consistently defined across environments
# Expected: Similar configuration in dev/prod environments
rg -g "values*.yaml" "SI_PUBLIC_GRAPHQL_PATH:" charts/
Length of output: 508
Line range hint 1-5: Warning: This is an auto-generated file
The header indicates this file is automatically generated. Any changes made directly to this file might be overwritten.
Let's verify the generation process:
✅ Verification successful
Warning: This file is auto-generated - direct modifications will be lost
The verification confirms that this file is indeed automatically generated by the yarn charts command, which runs the script at infra/src/cli/generate-chart-values.ts. The script explicitly adds the warning header to all generated chart values files. Any manual changes to this file will be overwritten the next time charts are regenerated.
To make changes:
- Modify the chart generation logic in
infra/src/cli/generate-chart-values.ts - Run
yarn chartsto regenerate all chart value files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for the chart generation script
# Expected: Find the script that generates this file
fd -g "package.json" | xargs rg "charts"
Length of output: 225
Script:
#!/bin/bash
# Let's check the content of the chart generation script
cat infra/src/cli/generate-chart-values.ts
Length of output: 2551
27-27: Consider documenting the empty GraphQL path behavior
An empty GraphQL path configuration might affect the service's behavior. Consider documenting:
- The purpose of this configuration
- The implications of an empty value
- How it affects the GraphQL endpoint routing
Let's check for related documentation:
charts/services/application-system-form/values.prod.yaml (2)
27-27: LGTM! The empty environment variable is valid.
The addition of SI_PUBLIC_GRAPHQL_PATH with an empty value is a valid configuration in both Kubernetes and Helm contexts.
Let's verify the consistency of this configuration across environments:
✅ Verification successful
Configuration is consistent across all environments
The SI_PUBLIC_GRAPHQL_PATH environment variable is properly defined with an empty value across all environments (dev, staging, and prod), maintaining consistency in the configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if SI_PUBLIC_GRAPHQL_PATH is consistently defined across environments
# Expected: The variable should be present in all environment files
# Search for the variable in all values files for this service
fd --type f "values\.(dev|staging|prod)\.yaml$" "charts/services/application-system-form" \
--exec grep -l "SI_PUBLIC_GRAPHQL_PATH"
# Show the actual values across environments
echo "Values across environments:"
fd --type f "values\.(dev|staging|prod)\.yaml$" "charts/services/application-system-form" \
--exec sh -c 'echo "=== $1 ==="; grep "SI_PUBLIC_GRAPHQL_PATH" "$1" || echo "Not found"' sh {}
Length of output: 836
Line range hint 1-5: Note: This is an auto-generated file
The warning header indicates this file should not be edited manually. Ensure your changes are made through the appropriate workflow using "yarn charts".
Let's verify the generation process:
✅ Verification successful
Warning: This file is auto-generated - manual changes will be overwritten
The verification confirms that this file is automatically generated by the yarn charts command. The generation process is handled by a dedicated script in infra/src/cli/generate-chart-values.ts which:
- Generates values for all Helm charts and services
- Automatically adds the warning header to all generated files
- Creates consistent YAML configurations across the system
Manual changes to values.prod.yaml will be lost on the next generation. Always use yarn charts to update these files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for chart generation documentation or scripts
# Expected: Find information about the chart generation process
# Look for documentation about chart generation
fd -t f "README\.md$" . -X grep -l "yarn charts"
# Look for the chart generation script
fd -t f "package\.json$" . -X grep -l "\"charts\":"
Length of output: 171
Script:
#!/bin/bash
# Let's check the content of the found files to understand the chart generation process
# Check the main README for yarn charts instructions
cat ./apps/judicial-system/README.md | grep -A 5 -B 5 "yarn charts"
# Check package.json files for the charts script definition
cat ./package.json | jq '.scripts.charts'
cat ./infra/package.json | jq '.scripts.charts'
# Look for any chart generation related files
fd -t f ".*charts.*\.(js|ts|sh)$"
Length of output: 760
Script:
#!/bin/bash
# Let's check the chart generation script implementation
cat infra/src/cli/generate-chart-values.ts
# Also check if there are any other files that generate values.prod.yaml
fd -t f "values\.prod\.yaml$" charts -X grep -l "automatically generated"
Length of output: 5609
infra/src/cli/generate-chart-values.ts (2)
34-34: LGTM! Function signature change is purely stylistic
The change from a regular function to an arrow function maintains the same functionality while following modern JavaScript conventions.
27-27: Verify the impact of allowing empty environment variables
The change to directly assign content without filtering empty properties aligns with the PR objective. However, let's verify that this doesn't affect any dependent services negatively.
✅ Verification successful
Based on the search results, I can see that environment variables are primarily used for configuration and there are no strict validations that would prevent empty environment variables from being used. The codebase appears to handle environment variables in a safe way by:
- Using fallbacks with
??operator - Using optional() in config modules
- Having default values for most environment variables
The change to allow empty environment variables in helm values is safe
The removal of empty property filtering aligns with the codebase's approach to environment variable handling. The codebase already has robust fallback mechanisms in place and no critical dependencies on non-empty environment variables were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential issues with empty environment variables in service configurations
# Look for environment variable validations in the codebase that might conflict with empty values
rg -A 3 "process\.env\.[A-Z_]+"
# Check for environment variable assertions or required checks
ast-grep --pattern 'if (!process.env.$_) { $_$ }'
Length of output: 100111
charts/services/judicial-system-api/values.dev.yaml (1)
33-33: LGTM! The empty environment variable is properly configured.
The addition of HIDDEN_FEATURES with an empty string value aligns with the PR objective to allow empty environment variables in Helm values.
Let's verify the source of this generated file:
✅ Verification successful
LGTM! The empty HIDDEN_FEATURES value is properly configured across environments
The HIDDEN_FEATURES environment variable is:
- Defined in the infrastructure code (
judicial-system-api.ts) - Consistently set to empty string across all environments (dev, staging, prod)
- Used by the FeatureModule to manage feature flags
- Generated automatically via the chart generation script with proper warning headers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find the template or source that generates this values file
# Expected: Find references to chart generation or templates
# Search for chart generation scripts
fd -t f "package.json" | xargs rg -l "charts"
# Search for references to judicial-system-api chart templates
fd -t f . | rg -l "judicial-system-api.*\.yaml\.j2|judicial-system-api.*\.yaml\.template"
Length of output: 167
Script:
#!/bin/bash
# Let's try a different approach to find the source of this generated file
# Check the content of package.json files for chart-related scripts
echo "=== Checking package.json files for chart generation scripts ==="
cat infra/package.json package.json
# Look for any files that might be related to chart generation
echo -e "\n=== Looking for files related to chart generation ==="
fd -t f . | rg -i "chart|helm|template|values"
# Check if there's a warning header in the values file indicating how it's generated
echo -e "\n=== Checking the warning header in values.dev.yaml ==="
head -n 5 charts/services/judicial-system-api/values.dev.yaml
Length of output: 151251
Script:
#!/bin/bash
# Let's check if this is a generated file and how it's generated
# Check if there's a script that generates chart values
echo "=== Checking chart generation script ==="
cat infra/src/cli/generate-chart-values.ts || echo "File not found"
# Check if there are any other references to HIDDEN_FEATURES
echo -e "\n=== Looking for other references to HIDDEN_FEATURES ==="
rg "HIDDEN_FEATURES" -A 2 -B 2
Length of output: 6541
charts/services/judicial-system-api/values.prod.yaml (2)
33-33: Implementation looks good!
The addition of HIDDEN_FEATURES: '' aligns with the PR objective of allowing empty environment variables in Helm values. This is a valid approach for feature flag configuration in Kubernetes.
33-33: Verify environment consistency
Let's ensure this configuration is consistent across all environments.
✅ Verification successful
Configuration is consistent across environments ✅
The HIDDEN_FEATURES configuration is consistently set to an empty string ('') across all environments (dev, staging, and prod) in the judicial-system-api service. No inconsistencies were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if HIDDEN_FEATURES is consistently defined across environments
# Expected: Should find HIDDEN_FEATURES in dev, staging, and prod values files
echo "Checking HIDDEN_FEATURES across environments:"
rg "HIDDEN_FEATURES:" "charts/services/judicial-system-api/values.*.yaml" "charts/judicial-system/values.*.yaml"
Length of output: 380
Script:
#!/bin/bash
# Let's first find the correct paths for the values files
echo "Finding values files:"
fd "values.*\.yaml" charts/
# Then check for HIDDEN_FEATURES in those files
echo -e "\nChecking HIDDEN_FEATURES in found files:"
fd "values.*\.yaml" charts/ --exec rg "HIDDEN_FEATURES:" {}
Length of output: 9423
Script:
#!/bin/bash
# Let's specifically check the judicial-system-api values files
echo "Checking HIDDEN_FEATURES in judicial-system-api values files:"
rg -A 1 "HIDDEN_FEATURES:" charts/services/judicial-system-api/values.{dev,staging,prod}.yaml
Length of output: 922
charts/services/judicial-system-api/values.staging.yaml (1)
33-33: LGTM: Empty environment variable implementation
The addition of HIDDEN_FEATURES: '' is correctly implemented as an empty string, which aligns with the PR objective to allow empty environment variables in Helm values. The placement maintains proper alphabetical ordering within the env section.
Let's verify if this pattern is used elsewhere in the codebase:
✅ Verification successful
Empty environment variables are consistently used across the codebase
The verification shows that empty environment variables are a common pattern across the codebase, particularly for feature flags and configuration settings. The implementation of HIDDEN_FEATURES: '' in judicial-system-api follows the same pattern used extensively throughout the services, with similar empty string values for environment variables like SERVERSIDE_FEATURES_ON and others.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for similar empty environment variable patterns
# Expected: Find other instances of intentionally empty env vars
# Search for empty string env vars in Helm values
echo "Checking for similar empty env vars patterns..."
rg ":\\s*''" charts/services/ --type yaml
Length of output: 12426
charts/services/judicial-system-backend/values.dev.yaml (4)
22-22: LGTM: Empty environment variable addition
The addition of BLOCKED_API_INTEGRATION: '' aligns with the PR objective to allow empty environment variables in Helm values. The placement and format are consistent with existing environment variable definitions.
22-22: Verify configuration consistency across environments
Please ensure the BLOCKED_API_INTEGRATION variable is appropriately configured across all relevant environments (dev, staging, prod).
22-22: Verify security implications of the new environment variable
Since BLOCKED_API_INTEGRATION appears to be related to API control, please ensure:
- Empty value doesn't unintentionally enable blocked integrations
- Default empty state is the secure default
✅ Verification successful
Let me analyze the usage of BLOCKED_API_INTEGRATION from the search results. The key findings show:
-
The variable is used in two places for API control:
courtClient.config.ts: Controls court API availabilitypolice.config.ts: Controls police case API availability
-
The empty value behavior:
- Empty value leads to
!('')which evaluates totrue, meaning APIs are available - Non-empty values like
'COURT,POLICE_CASE'block specific integrations
- Empty value leads to
-
Environment configurations:
- Production/Dev: Empty value (
'') - APIs enabled - Staging:
'COURT,POLICE_CASE'- APIs blocked
- Production/Dev: Empty value (
Empty value is the secure default as it maintains API access in production
The empty value in dev environment aligns with production configuration and represents the intended state where APIs are accessible. The variable is used as a feature flag to selectively disable integrations in non-production environments (staging) rather than as a security control.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of BLOCKED_API_INTEGRATION in the codebase
rg -A 5 "BLOCKED_API_INTEGRATION"
# Look for any security-related patterns around API blocking
ast-grep --pattern 'if ($BLOCKED_API_INTEGRATION) { $$$ }'
Length of output: 124158
Line range hint 1-6: Verify the generation process for this change
The file header indicates it's automatically generated. Ensure you've used the proper tooling (yarn charts) to make this change rather than editing the file directly.
charts/services/judicial-system-backend/values.prod.yaml (3)
Line range hint 19-54: Configuration structure looks good
The new environment variable follows the established pattern and maintains the clear separation between environment variables and secrets.
Line range hint 1-156: Verify consistency across environments
Since this is a production configuration file, we should ensure consistent handling of empty environment variables across all environments.
Let's check for consistency:
22-22: Document the purpose of BLOCKED_API_INTEGRATION
While adding an empty environment variable aligns with the PR objective, please document:
- The purpose of this variable
- The impact of an empty value
- Which API integrations are affected
Let's check how this variable is used in the codebase:
✅ Verification successful
BLOCKED_API_INTEGRATION is used to control API availability
The empty value in production is correct. This environment variable:
- Controls the availability of Court and Police Case APIs
- Uses a comma-separated list of services to block (e.g., 'COURT,POLICE_CASE')
- Empty value ('') means no APIs are blocked, allowing all integrations to function
Evidence from the codebase:
- Used in
courtClient.config.tsto control Court API availability - Used in
police.config.tsto control Police Case API availability - Staging environment blocks both APIs with 'COURT,POLICE_CASE'
- Development and production environments have it empty to enable all integrations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for BLOCKED_API_INTEGRATION usage
rg -A 5 "BLOCKED_API_INTEGRATION" --type typescript --type javascript
Length of output: 104
Script:
#!/bin/bash
# Search for BLOCKED_API_INTEGRATION usage with ripgrep, without file type restrictions
rg -A 5 "BLOCKED_API_INTEGRATION"
# Also search for any files containing "api" and "integration" to understand the context
fd -t f . | rg -i "api.*integration|integration.*api"
Length of output: 5048
infra/src/dsl/service-to-environment/pre-process-service.ts (2)
Line range hint 239-276: LGTM: Robust environment variable handling
The getEnvValue function implementation correctly handles:
- Missing settings
- Undefined values for local environment
- Empty environment variables
This aligns well with the PR objective to allow empty envs in helm values.
321-326: Type safety improvement in feature filtering
The refactoring improves type safety by explicitly asserting the types of feature entries. This ensures proper handling of feature configurations and their environment variables.
Let's verify the handling of empty environment variables:
✅ Verification successful
Let me run another script to check the TypeScript code handling of empty environment variables:
Empty environment variables are properly handled in the codebase
The code changes in pre-process-service.ts are safe for handling empty environment variables because:
- The
MissingSettingtype is used to explicitly handle missing values, with proper error handling in place - The feature filtering code maintains type safety while allowing empty or undefined values
- The Helm templates have explicit checks for empty values using
emptyconditionals
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for empty environment variable handling in helm values
# Look for patterns that might prevent empty env vars
# Search for environment variable validation or constraints
rg -A 3 "MissingSetting|getEnvValue|env\." --type typescript
# Look for helm value templates that might enforce non-empty values
fd . -e yaml -x grep -l "empty" {} \; | xargs cat
Length of output: 22879
Script:
#!/bin/bash
# Search for environment variable handling in TypeScript files
ast-grep --pattern 'env.$_'
# Search for getEnvValue function implementation
ast-grep --pattern 'function getEnvValue($_) {
$$$
}'
# Search for MissingSetting error handling
rg "MissingSetting" -A 3
Length of output: 92725
infra/src/dsl/output-generators/map-to-helm-values.ts (2)
348-350: LGTM! Clean type handling for ingress hosts.
The type handling for ingress hosts is well-implemented, properly handling both string and array cases.
Line range hint 78-434: Verify the impact on Helm chart rendering.
The changes consistently improve the handling of optional values across the codebase. However, we should verify that these changes don't affect the rendering of Helm charts when values are undefined.
✅ Verification successful
Optional healthPort handling is implemented correctly
Based on the code inspection, the changes to handle optional healthPort are properly implemented:
- The healthCheck object is always initialized with required liveness and readiness properties
- The healthPort is conditionally added only when serviceDef.healthPort is defined
- The implementation maintains backward compatibility by keeping the same structure
The code shows a clear pattern for handling optional values:
healthCheck: {
// port is conditionally added later
liveness: { /* required fields */ },
readiness: { /* required fields */ }
}
if (serviceDef.healthPort) {
result.healthCheck.port = serviceDef.healthPort
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential issues in Helm chart rendering with undefined values
# Search for Helm template files that might be affected
fd -e yaml -e tpl . charts/ -x echo "Checking {}:" \; -x grep -A 5 -B 5 'port:|image:|host:' {} \;
# Search for related test files
fd -e ts -e js test -x echo "Found test file {}:" \; -x grep -A 5 -B 5 'healthPort|image|host' {} \;
Length of output: 17950
Script:
#!/bin/bash
# Let's verify the Helm chart templates and values handling
# Check Helm templates for handling of optional values
ast-grep --pattern 'if $value $$$' charts/ || true
# Search for template files that might use these values
fd -e yaml -e tpl . charts/ -x echo "File: {}" \; -x cat {} \; | grep -A 5 -B 5 '{{.*port.*}}|{{.*healthCheck.*}}|{{.*image.*}}|{{.*host.*}}'
# Check test files for related test cases
ast-grep --pattern 'describe($_, function() {
$$$
test($_, function() {
$$$
healthPort
$$$
})
$$$
})' || true
Length of output: 316
Script:
#!/bin/bash
# Let's check the test coverage and implementation for these changes
# Search for test files that verify the handling of optional values
rg -l "healthPort|healthCheck" test/ -g "*.test.ts" -g "*.spec.ts"
# Check the implementation of map-to-helm-values
rg -A 10 -B 10 "healthPort|healthCheck" infra/src/dsl/output-generators/map-to-helm-values.ts
# Look for related test cases in the infrastructure tests
rg -A 10 -B 10 "healthPort|healthCheck" infra/src/dsl/output-generators/__tests__/
Length of output: 2188
charts/judicial-system/values.dev.yaml (2)
Line range hint 1-24: Security configurations look appropriate
The file maintains good security practices:
- Proper security contexts with
privileged: false - Well-defined resource limits and requests
- Sensitive values stored in secrets
33-33: Verify empty environment variables handling
The addition of empty environment variables (HIDDEN_FEATURES and BLOCKED_API_INTEGRATION) aligns with the PR objective. However, we should ensure that the application code properly handles these empty values.
Also applies to: 109-109
✅ Verification successful
Empty environment variables are properly handled in the codebase
The codebase safely handles both empty environment variables:
HIDDEN_FEATURESis processed usingsplitStringByComma(env.optional('HIDDEN_FEATURES')), which safely handles empty values through theoptionalmethodBLOCKED_API_INTEGRATIONis used with null coalescing (??) and string splitting, ensuring empty values default to an empty array after the split operation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how these environment variables are used in the codebase
# Look for usage of these variables in the application code
echo "Checking HIDDEN_FEATURES usage:"
rg -A 3 "HIDDEN_FEATURES" --type ts --type js
echo "Checking BLOCKED_API_INTEGRATION usage:"
rg -A 3 "BLOCKED_API_INTEGRATION" --type ts --type js
Length of output: 2045
charts/judicial-system/values.staging.yaml (2)
33-33: LGTM: Environment variable addition aligns with PR objective
The addition of HIDDEN_FEATURES='' as an empty environment variable is consistent with the PR's goal to allow empty environment variables in Helm values.
Line range hint 1-7: Verify changes are made through the chart generation process
The file header indicates this is an automatically generated file. Please ensure these changes are made through the proper chart generation process using yarn charts rather than direct edits.
✅ Verification successful
Changes follow the proper generation process
The infrastructure file (judicial-system-api.ts) shows that HIDDEN_FEATURES is properly defined in the service configuration with empty string values for all environments, including staging. This matches the documentation which instructs to use yarn charts for generating the values files. The changes are being made through the correct infrastructure-as-code approach rather than direct manual edits to the values files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if the changes are properly generated
# Look for chart generation configuration files
fd -t f "values.yaml$" charts/judicial-system -x echo "Found chart values file: {}"
Length of output: 85
Script:
#!/bin/bash
# Let's try a broader search for Helm-related files and chart generation scripts
echo "=== Searching for Helm chart files ==="
fd -t f "\.yaml$" charts/judicial-system
echo -e "\n=== Searching for chart generation scripts ==="
fd -t f "package\.json$" . -x grep -l "charts"
echo -e "\n=== Looking for chart generation documentation ==="
fd -t f "README\.md$" . -x grep -A 5 -i "chart" {}
Length of output: 2359
Script:
#!/bin/bash
# Let's check the specific file mentioned in the documentation
echo "=== Checking for the infrastructure file ==="
cat apps/judicial-system/api/infra/judicial-system-api.ts 2>/dev/null || echo "File not found"
echo -e "\n=== Checking the actual changes in the values file ==="
git diff charts/judicial-system/values.staging.yaml || echo "No diff available"
Length of output: 3872
charts/judicial-system/values.prod.yaml (3)
Line range hint 153-157: Verify scaling behavior with reduced nginxRequestsIrate
The HPA configuration for judicial-system-backend has been adjusted to use a lower nginxRequestsIrate (5 instead of 8), which will trigger scaling at a lower request rate threshold. While this aligns with other services in the configuration, please verify:
- Historical metrics to ensure this won't cause unnecessary scaling
- Cost implications of potentially more frequent scaling events
Line range hint 1-24: Configuration follows security best practices
The overall configuration demonstrates good security practices:
- Sensitive information stored in AWS Secrets Manager
- Proper security contexts with privilege restrictions
- Consistent resource limits and requests
33-33: Document the purpose of new environment variables
Two new environment variables have been added with empty values. While allowing empty values is the intended behavior, please document their purpose and impact:
HIDDEN_FEATURES: What features can be controlled through this variable?BLOCKED_API_INTEGRATION: What integrations can be blocked using this variable?
Also applies to: 109-109
✅ Verification successful
Environment variables are well documented in the codebase
The search results show clear usage and purpose of both environment variables:
HIDDEN_FEATURES: Used in the feature configuration module to control feature flags. The README.md provides instructions for adding secret features.BLOCKED_API_INTEGRATION: Used to disable specific integrations by name (e.g., 'COURT', 'POLICE_CASE'). The code shows it's used to control availability of court and police case API integrations.
Both variables are properly implemented with empty string defaults and consistent usage across environments.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for documentation or usage of these variables
echo "Searching for HIDDEN_FEATURES usage..."
rg -A 3 "HIDDEN_FEATURES"
echo "Searching for BLOCKED_API_INTEGRATION usage..."
rg -A 3 "BLOCKED_API_INTEGRATION"
Length of output: 6753
charts/services/api/values.prod.yaml (2)
55-55: LGTM! Environment variable addition aligns with PR objectives.
The addition of FORM_SYSTEM_API_BASE_PATH with an empty string value is consistent with the PR's goal of allowing empty environment variables in Helm values.
55-55: Verify the impact of empty base path on form system functionality.
An empty base path value might affect how the API service interacts with the form system. Please ensure that:
- Form system API calls work correctly with a relative path
- This change doesn't unintentionally disable form system integration
- The empty value is the intended default for the production environment
✅ Verification successful
Empty base path is intentionally used in staging environment
The empty base path value is consistent with the staging environment configuration and appears to be intentional. The form system client has a default fallback URL (https://profun.island.is/umsoknarkerfi) if no value is provided, ensuring the system remains functional. This pattern is already established in the staging environment, indicating it's a tested configuration.
FormSystemClient.config.ts: Has a default URL configured if the environment variable is emptyapi.ts: Shows empty base path is already used in staging environment
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for form system API usage to understand the impact
echo "Searching for form system API usage patterns..."
rg -l "FORM_SYSTEM_API_BASE_PATH" --type ts --type js
# Look for any form system URL construction logic
ast-grep --pattern 'new URL($_, process.env.FORM_SYSTEM_API_BASE_PATH)'
Length of output: 314
Script:
#!/bin/bash
# Check the implementation in FormSystemClient.config.ts
echo "=== FormSystemClient.config.ts ==="
rg "FORM_SYSTEM_API_BASE_PATH" libs/clients/form-system/src/lib/FormSystemClient.config.ts -B 2 -A 2
echo -e "\n=== api.ts configuration ==="
rg "FORM_SYSTEM_API_BASE_PATH" apps/api/infra/api.ts -B 2 -A 2
# Look for URL construction or path joining logic
echo -e "\n=== URL construction patterns ==="
ast-grep --pattern 'join($_, process.env.FORM_SYSTEM_API_BASE_PATH)'
ast-grep --pattern 'path.join($_, process.env.FORM_SYSTEM_API_BASE_PATH)'
Length of output: 867
charts/services/api/values.staging.yaml (3)
Line range hint 6-7: LGTM: Proper documentation header
The file includes a clear header indicating it's automatically generated and warning against manual edits.
Line range hint 419-450: LGTM: Secure pod configuration
The security context configuration properly restricts privilege escalation and privileged mode, following Kubernetes security best practices.
55-55: Verify the intentional empty value configuration
The newly added environment variable FORM_SYSTEM_API_BASE_PATH is set to an empty string. While this aligns with the PR objective to allow empty environment variables, please ensure this is the intended configuration for the staging environment.
✅ Verification successful
Empty value for FORM_SYSTEM_API_BASE_PATH is consistent with production configuration
The empty value configuration for FORM_SYSTEM_API_BASE_PATH in values.staging.yaml is correct as it matches the production configuration:
- Empty value is set in
charts/services/api/values.prod.yaml - Empty value is also used in
charts/islandis/values.prod.yaml - Only development environments (
values.dev.yaml) use the URL 'https://profun.island.is/umsoknarkerfi'
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if this environment variable is defined differently in other environments
echo "Checking FORM_SYSTEM_API_BASE_PATH configuration across environments..."
rg "FORM_SYSTEM_API_BASE_PATH:" charts/
Length of output: 680
charts/islandis/values.staging.yaml (2)
306-306: Verify the impact of empty FORM_SYSTEM_API_BASE_PATH
The empty value for FORM_SYSTEM_API_BASE_PATH environment variable might affect form system API routing. Please ensure this is intentional and won't break existing form submissions.
✅ Verification successful
Empty FORM_SYSTEM_API_BASE_PATH is consistent with production configuration
The empty value for FORM_SYSTEM_API_BASE_PATH in staging environment matches the production configuration. This is intentional as both staging (charts/islandis/values.staging.yaml) and production (charts/islandis/values.prod.yaml) environments use an empty base path, while only the development environment uses 'https://profun.island.is/umsoknarkerfi'. This pattern is also consistently defined in the infrastructure code (apps/api/infra/api.ts).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any hardcoded form system API paths that might be affected
# Test: Search for form system API path references that might need the base path
rg -A 3 "formSystemApi|FORM_SYSTEM_API"
Length of output: 3257
933-933: Verify GraphQL endpoint configuration
The empty SI_PUBLIC_GRAPHQL_PATH might affect GraphQL client configurations in the application system form. Ensure this aligns with your GraphQL endpoint setup.
✅ Verification successful
Looking at the results, I can generate the final response as we have sufficient information to verify the GraphQL endpoint configuration.
Empty SI_PUBLIC_GRAPHQL_PATH is the expected configuration
The empty value for SI_PUBLIC_GRAPHQL_PATH is correct and consistent across all environments (dev, staging, prod) in both the islandis and application-system-form charts. This configuration works in conjunction with the environment.ts file in the application system form, which falls back to window.location.origin when SI_PUBLIC_GRAPHQL_PATH is empty:
baseApiUrl: getStaticEnv('SI_PUBLIC_GRAPHQL_PATH') ?? window.location.originThis setup allows the application to use the same origin for API requests, which is a common and secure pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for GraphQL path usage in application system
# Test: Look for GraphQL client configurations that might be affected
ast-grep --pattern 'graphql($_, $_)'
rg -A 3 "graphqlPath|GRAPHQL_PATH"
Length of output: 9478
charts/islandis/values.prod.yaml (2)
924-924: Verify the empty GraphQL path configuration
The empty value for SI_PUBLIC_GRAPHQL_PATH suggests the GraphQL endpoint will be at the root path. This should be consistent with:
- The API service's GraphQL endpoint configuration
- The client-side GraphQL configuration
✅ Verification successful
Empty GraphQL path configuration is consistent across environments
The empty value for SI_PUBLIC_GRAPHQL_PATH is consistently configured across all environments:
- In islandis charts (dev/staging/prod)
- In application-system-form service (dev/staging/prod)
This configuration coexists with:
SI_PUBLIC_GRAPHQL_API: '/api/graphql'for service-portalGRAPHQL_API_URL: 'http://web-api.islandis.svc.cluster.local'for internal service communication
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check GraphQL path configurations across services
rg "GRAPHQL_" charts/
Length of output: 1623
294-294: Verify the empty base path configuration for Form System API
The empty value for FORM_SYSTEM_API_BASE_PATH indicates that the Form System API will be served from the root path. Please ensure this aligns with:
- The API Gateway/Ingress configuration
- Other services' base paths to avoid routing conflicts
✅ Verification successful
Empty base path configuration for Form System API is consistent with environment setup
The empty value for FORM_SYSTEM_API_BASE_PATH in production is intentional and aligns with the configuration across different environments:
- Development uses a specific base path:
https://profun.island.is/umsoknarkerfi - Staging and Production use an empty base path:
''
This configuration is consistently applied in both the API service (charts/services/api/values.prod.yaml) and the main islandis chart (charts/islandis/values.prod.yaml). The ingress configurations show no path conflicts that would affect this setup.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential path conflicts in ingress configurations
rg -A 5 "path: '/'" charts/
Length of output: 33322
Script:
#!/bin/bash
# Let's check for ingress configurations and API paths to verify routing
rg -A 10 "ingress:|path:" charts/islandis/values.prod.yaml
Length of output: 18227
Script:
#!/bin/bash
# Let's check for API routes and base paths in the codebase
rg -A 5 "FORM_SYSTEM_API_BASE_PATH|api.routes" .
Length of output: 6040
charts/islandis/values.dev.yaml (1)
936-936: LGTM: Empty environment variable aligns with PR objective
The addition of SI_PUBLIC_GRAPHQL_PATH with an empty string value is valid and matches the goal of allowing empty environment variables in Helm values.
… into fix/allow-empty-envs
busla
added
automerge
Allow setting empty env vars in infra DSL
What
This PR fixes changes made in #16869 where all empty values were removed in the serialized helm values files. This resulted in breaking changes when empty env vars were removed but required for applications.
Now we allow empty values by improving the DSL by checking if attributes (e.g.
portandinitContainer.containers[].image) are empty and ignoring them if so.Why
Specify why you need to achieve this
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
New Features
SI_PUBLIC_GRAPHQL_PATH,FORM_SYSTEM_API_BASE_PATH,HIDDEN_FEATURES, andBLOCKED_API_INTEGRATION.Bug Fixes
Chores