feat: ✨ add applicaiton config #7
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces several changes across various files, focusing on the addition of environment variable configurations and updates to API responses. The Changes
Possibly related PRs
Warning Rate limit exceeded@github-actions[bot] has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 29 minutes and 28 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (8)
libs/application-config/src/lib/applications-config.spec.ts (1)
4-4: Use a more descriptive test name.The test description "should work" is not descriptive enough. Consider renaming it to better describe what is being tested.
- it('should work', () => { + it('should return the module name', () => {apps/users-application-e2e/src/users-application/users-application.spec.ts (2)
5-5: Simplify the template literal.The template literal isn't interpolating any variables. Consider using a string literal instead.
- const res = await axios.get(`/`); + const res = await axios.get('/');
3-9: Consider adding error handling test cases.The test suite only covers the happy path. Consider adding test cases for:
- Invalid routes (404)
- Server errors (500)
- Network errors
Example addition:
it('should handle invalid routes', async () => { try { await axios.get('/invalid-route'); fail('Expected request to fail'); } catch (error) { expect(error.response.status).toBe(404); } });apps/users-application/src/main.ts (1)
Line range hint
1-22: Consider adding graceful shutdown handling.As this is a GraphQL Federation service, it's important to handle shutdown gracefully to prevent disruption to the federated graph.
Consider adding:
- Signal handlers for graceful shutdown
- Health check endpoint for container orchestration
- Logging of shutdown events
Here's a suggested implementation:
async function bootstrap() { const app = await NestFactory.create(AppModule); const port = userSubGraph.port ?? 3000; // Add health check app.get('/health', (req, res) => res.send('OK')); // Graceful shutdown const signals = ['SIGTERM', 'SIGINT']; signals.forEach(signal => { process.on(signal, async () => { Logger.log(`Received ${signal}, starting graceful shutdown`); await app.close(); process.exit(0); }); }); await app.listen(port); Logger.log( `🚀 Application is running on: http://${userSubGraph.host}:${port}` ); }apps/gateway/src/main.ts (2)
8-8: LGTM! Consider adding type information.The import of
gatewayConfigaligns with the PR objective of adding application configuration.Consider adding a type declaration file for the config to ensure type safety:
// applications-config.d.ts export interface GatewayConfig { host: string; port: number; }
Line range hint
1-22: Consider adding startup configuration logging.For better operational visibility, consider logging the non-sensitive configuration values during startup.
Add configuration logging before starting the server:
async function bootstrap() { const app = await NestFactory.create(AppModule); const port = gatewayConfig.port; + Logger.log(`Starting gateway with configuration: + - Host: ${gatewayConfig.host} + - Port: ${port} + `); await app.listen(port); Logger.log( `🚀 Application is running on: http://${gatewayConfig.host}:${port}` ); }libs/application-config/src/lib/applications-config.ts (2)
15-24: Refactor subgraph configurations to reduce duplication.The configuration pattern is repeated across services. Consider creating a factory function.
+function createServiceConfig( + serviceName: string, + hostEnvVar: string, + portEnvVar: string, + defaultPort: number +): ServiceConfig { + return { + host: process.env[hostEnvVar] || DEFAULT_HOST, + port: (() => { + const port = parseInt(process.env[portEnvVar] || String(defaultPort), 10); + if (!validatePort(port)) { + throw new Error(`Invalid ${serviceName} port: ${port}`); + } + return port; + })(), + }; +} + -export const userSubGraph = { - host: process.env['USER_HOST'] ?? DEFAULT_HOST, - port: process.env['USER_PORT'] ?? DEFAULT_PORT.user, -}; +export const userSubGraph = createServiceConfig( + 'user', + 'USER_HOST', + 'USER_PORT', + DEFAULT_PORT.user +); -export const taskSubGraph = { - host: process.env['TASK_HOST'] ?? DEFAULT_HOST, - port: process.env['TASK_PORT'] ?? DEFAULT_PORT.task, -}; +export const taskSubGraph = createServiceConfig( + 'task', + 'TASK_HOST', + 'TASK_PORT', + DEFAULT_PORT.task +);
15-24: Add documentation for service configurations.Please add JSDoc comments to describe the purpose and requirements of each service configuration.
+/** + * Configuration for the User service subgraph. + * This service handles user-related operations in the GraphQL federation. + * @requires USER_HOST - Environment variable for user service host + * @requires USER_PORT - Environment variable for user service port + */ export const userSubGraph = { // ... existing code }; +/** + * Configuration for the Task service subgraph. + * This service handles task-related operations in the GraphQL federation. + * @requires TASK_HOST - Environment variable for task service host + * @requires TASK_PORT - Environment variable for task service port + */ export const taskSubGraph = { // ... existing code };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (23)
.env.sample(1 hunks).gitignore(1 hunks)apps/gateway-e2e/src/gateway/gateway.spec.ts(1 hunks)apps/gateway/src/app/app.controller.spec.ts(1 hunks)apps/gateway/src/app/app.service.spec.ts(1 hunks)apps/gateway/src/app/app.service.ts(1 hunks)apps/gateway/src/main.ts(1 hunks)apps/users-application-e2e/src/users-application/users-application.spec.ts(1 hunks)apps/users-application/src/app/app.controller.spec.ts(1 hunks)apps/users-application/src/app/app.service.spec.ts(1 hunks)apps/users-application/src/app/app.service.ts(1 hunks)apps/users-application/src/main.ts(1 hunks)libs/application-config/README.md(1 hunks)libs/application-config/eslint.config.js(1 hunks)libs/application-config/jest.config.ts(1 hunks)libs/application-config/project.json(1 hunks)libs/application-config/src/index.ts(1 hunks)libs/application-config/src/lib/applications-config.spec.ts(1 hunks)libs/application-config/src/lib/applications-config.ts(1 hunks)libs/application-config/tsconfig.json(1 hunks)libs/application-config/tsconfig.lib.json(1 hunks)libs/application-config/tsconfig.spec.json(1 hunks)tsconfig.base.json(1 hunks)
✅ Files skipped from review due to trivial changes (12)
- .env.sample
- .gitignore
- apps/gateway/src/app/app.service.ts
- apps/users-application/src/app/app.service.ts
- libs/application-config/README.md
- libs/application-config/eslint.config.js
- libs/application-config/jest.config.ts
- libs/application-config/project.json
- libs/application-config/src/index.ts
- libs/application-config/tsconfig.json
- libs/application-config/tsconfig.lib.json
- libs/application-config/tsconfig.spec.json
🔇 Additional comments (12)
apps/gateway-e2e/src/gateway/gateway.spec.ts (2)
8-8: LGTM: Consistent response message update.
The updated expected response message matches the changes made in other test files, maintaining consistency across the codebase.
5-5:
Configure axios with gateway host and port.
The current axios request to / without a base URL might not work correctly in an e2e environment. Consider using the newly added environment variables to properly configure the request URL.
Apply this diff to use the gateway configuration:
- const res = await axios.get(`/`);
+ const res = await axios.get(`http://${process.env.GATEWAY_HOST}:${process.env.GATEWAY_PORT}/`);Let's verify the environment variable configuration:
apps/users-application-e2e/src/users-application/users-application.spec.ts (1)
5-5: Verify base URL configuration for E2E tests.
The test is making requests to relative URLs. Ensure that the base URL is properly configured for E2E testing environment.
✅ Verification successful
Let me check the content of the test setup file to verify the axios configuration.
Base URL is properly configured for E2E tests
The axios base URL is correctly configured in test-setup.ts using environment variables with sensible defaults:
- Host: defaults to 'localhost'
- Port: defaults to '3000'
- Base URL format:
http://${host}:${port}
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for base URL configuration in test setup files
# Look for test setup files
echo "Searching for test setup files..."
fd -e ts -e js --full-path "setup|config|environment" apps/users-application-e2e
# Look for axios configuration
echo "Checking for axios configuration..."
rg -l "axios\.(defaults|create)" apps/users-application-e2e
Length of output: 549
Script:
#!/bin/bash
# Check the content of test setup file
cat apps/users-application-e2e/src/support/test-setup.ts
Length of output: 331
apps/gateway/src/app/app.service.spec.ts (2)
17-18: Consider updating API documentation if this is a public-facing message.
If this message is part of a public API response, ensure that the API documentation is updated to reflect this change.
Let's check for API documentation files that might need updates:
#!/bin/bash
# Description: Look for API documentation files that might need updates
# Expected: Find API documentation files that might reference the old message
fd -e md -e yaml -e json swagger
fd -e md -e yaml -e json openapi17-18: LGTM! Verify coordinated changes across services.
The test update looks good and follows proper testing practices. The change from "Hello API" to "Hello Gateway" appears to be part of a broader update across services.
Let's verify that this change is consistently applied across related files:
✅ Verification successful
✅ Change is consistently implemented across the Gateway service
The verification shows that:
- The message "Hello Gateway" is consistently used across all relevant files in the Gateway service:
app.service.ts: Implementationapp.service.spec.ts: Service testapp.controller.spec.ts: Controller test
- No remaining instances of "Hello API" were found in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent message updates across services
# Expected: All related files should use "Hello Gateway" instead of "Hello API"
echo "Checking for any remaining 'Hello API' references:"
rg -l "Hello API"
echo "\nVerifying 'Hello Gateway' implementation in service:"
rg -l "Hello Gateway" "apps/gateway/src/app/app.service.ts"
Length of output: 164
Script:
#!/bin/bash
# Let's try a more focused search approach
# Check for "Hello Gateway" pattern in app.service.ts
echo "Checking app.service.ts implementation:"
rg "Hello Gateway" apps/gateway/src/app/app.service.ts
# Check for any "Hello API" occurrences in the codebase
echo -e "\nChecking for any remaining 'Hello API' references:"
rg "Hello API"
# Look for other similar message patterns in gateway app
echo -e "\nChecking for similar message patterns in gateway app:"
rg "message.*Hello" apps/gateway/
Length of output: 795
apps/users-application/src/app/app.service.spec.ts (1)
17-18: LGTM! Test changes are consistent and well-implemented.
The updated test description and expectation align with the broader changes across the users application, maintaining consistency with the modifications in app.service.ts and app.controller.spec.ts.
apps/users-application/src/main.ts (2)
8-8: LGTM! Clean import of configuration.
The import of userSubGraph from the shared configuration library aligns well with the PR objective of adding application config.
18-18: Verify host configuration across environments.
The log message now uses dynamic host configuration which is good for flexibility, but we should ensure it's properly set across all environments.
✅ Verification successful
Host configuration is properly implemented with fallback values
The host configuration is well-structured with:
- Centralized configuration in
applications-config.ts - Environment variable support via
USER_HOSTwith fallback to 'localhost' - Consistent pattern across all services (user, task, gateway)
- Default values ensuring the application works out of the box
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for host configuration in environment files
# Expected: Find all environment files and verify USER_HOST is defined
echo "Checking environment files for host configuration..."
fd -t f -e env -e env.sample -e env.example -x grep -l "USER_HOST"
echo "Checking TypeScript config files for host references..."
fd -t f -e ts -e json -x grep -l "userSubGraph.host"
Length of output: 392
Script:
#!/bin/bash
# Let's check the implementation details of userSubGraph and host configuration
# Check how userSubGraph is defined and configured
echo "Checking userSubGraph implementation in main.ts:"
cat ./apps/users-application/src/main.ts
# Look for any environment configuration files
echo -e "\nLooking for environment files:"
fd -t f -e env -e env.example -e env.development -e env.production
# Search for any host-related configuration in the codebase
echo -e "\nSearching for host-related configuration:"
rg -l "host.*=|HOST.*=" --type-add 'config:*.{env,yaml,yml,json,ts}' -t config
Length of output: 1188
Script:
#!/bin/bash
# Let's check the host configuration in the application config and test files
echo "Checking applications-config.ts:"
cat libs/application-config/src/lib/applications-config.ts
echo -e "\nChecking test setup files:"
cat apps/users-application-e2e/src/support/test-setup.ts
cat apps/gateway-e2e/src/support/test-setup.ts
Length of output: 1421
apps/gateway/src/main.ts (1)
18-18: Verify host configuration across environments.
The logging message now uses dynamic host configuration.
Let's verify the host configuration setup:
✅ Verification successful
Let me gather more information about the host configuration setup.
Let me check one more thing to understand the default configuration.
Host configuration is properly implemented with fallback values
The host configuration is well-structured with:
- Environment variable
GATEWAY_HOSTdefined in.env.sample - Fallback to
localhostif not set - Consistent pattern across all services (gateway, user, task)
- Single source of truth in
applications-config.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify host configuration setup across the codebase
# Check for environment variable definitions
echo "Checking .env.sample for host configuration..."
cat .env.sample
# Check for host configuration usage
echo "Checking for other uses of host configuration..."
rg "gatewayConfig.host" --type ts
Length of output: 432
Script:
#!/bin/bash
# Check for host configuration implementation and usage
echo "Checking gateway configuration implementation..."
rg -A 5 "interface.*Config" --type ts
echo "Checking environment variable loading..."
rg "GATEWAY_HOST" --type ts
Length of output: 373
Script:
#!/bin/bash
# Check for default configuration
echo "Checking for default host configuration..."
rg "DEFAULT_HOST" --type ts -A 2
echo "Checking configuration module..."
cat libs/application-config/src/lib/applications-config.ts
Length of output: 1886
tsconfig.base.json (1)
17-21: Verify the application-config library path.
The path mapping looks correct and follows TypeScript module resolution best practices. Let's verify that the target path exists and is properly structured.
✅ Verification successful
Let me verify if this is a new library being set up by checking for recent changes and the library's structure.
Path mapping configuration is correct and library structure is valid
The library is properly set up with all necessary TypeScript configuration files and source files. The git history shows this is a new library with recent commits adding configuration constants. While package.json is missing, it's not critical as the library is managed through the workspace's project.json file, which is present.
- ✓ Source files exist at the mapped path
- ✓ Library has proper TypeScript configuration (tsconfig.json, tsconfig.lib.json)
- ✓ Project configuration exists (project.json)
- ✓ No duplicate path mappings found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of the application-config library and its structure
# Check if the library path exists
if [ -f "libs/application-config/src/index.ts" ]; then
echo "✓ Library path exists"
# Check if the library has proper structure
if [ -f "libs/application-config/package.json" ] && [ -f "libs/application-config/tsconfig.json" ]; then
echo "✓ Library has proper structure"
else
echo "⚠ Library might be missing package.json or tsconfig.json"
fi
else
echo "⚠ Library path not found: libs/application-config/src/index.ts"
fi
# Verify no duplicate path mappings
grep -r "@graphql-federation-workspace/applications-config" . --include="tsconfig*.json"
Length of output: 446
Script:
#!/bin/bash
# Check git history for the library setup
git log --stat --oneline -- libs/application-config/
# List all files in the library directory
find libs/application-config -type f
# Check if there are any pending changes
git status --porcelain -- libs/application-config/
Length of output: 1664
apps/gateway/src/app/app.controller.spec.ts (1)
17-17: LGTM! Test updates align with service changes.
The test modifications correctly reflect the new expected response from the gateway service, maintaining consistency with the related changes in app.service.ts and gateway.spec.ts.
Also applies to: 19-19
apps/users-application/src/app/app.controller.spec.ts (1)
17-17: LGTM! Test case properly updated.
The test case has been correctly updated to reflect the new expected response from the user service. The changes are consistent with the broader modifications mentioned in the PR summary, where the service response is being changed from "Hello API" to "Hello User Service".
Let's verify the consistency of this change with the related service implementation:
Also applies to: 19-19
✅ Verification successful
Test case correctly aligns with service implementation
The test expectation "Hello User Service" perfectly matches the actual implementation in app.service.ts where getData() returns { message: 'Hello User Service' }.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the service implementation matches the test expectations
# Expected: The service should return "Hello User Service" message
# Search for the getData implementation in the service file
rg -A 5 "getData.*{" "apps/users-application/src/app/app.service.ts"
Length of output: 152
Script:
#!/bin/bash
# Let's try a simpler pattern search without regex metacharacters
rg "getData" "apps/users-application/src/app/app.service.ts" -A 5
Length of output: 150
zhumeisongsong
changed the title
|
🎉 This PR is included in version 1.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
closes #5
Summary by CodeRabbit
Release Notes
New Features
.env.samplefile for environment variable configuration.applications-config.tsfile.Bug Fixes
Documentation
applications-configlibrary, outlining usage and testing instructions.Configuration
applications-configlibrary to enhance project setup and testing capabilities.Chores
.gitignoreto exclude.envand.env.localfiles.