-
Notifications
You must be signed in to change notification settings - Fork 0
Dev #68
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR centralizes and secures environment configuration, enhances email link construction in the ticketing service, and updates Terraform and Docker Compose to provision and distribute new email-related secrets and variables.
- Consolidate and strengthen
.env.templateacross services, addingAPP_BASE_URL - Augment email service to use dynamic base URLs and error logging
- Extend Terraform modules and Docker Compose with SendGrid and app-URL support
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| terraform/modules/ecs_cluster/variables.tf | Added variables for SendGrid key, sender address, and base URL |
| terraform/modules/ecs_cluster/main.tf | Granted IAM access to SendGrid secret; passed new env and secret entries into task definitions |
| terraform/main/variables.tf | Declared top-level sendgrid_api_key and email_from_address variables |
| terraform/main/main.tf | Provisioned SendGrid secret and passed its ARN plus email address and app URL into ECS module |
| docker-compose.yml | Exposed APP_BASE_URL in service environment blocks |
| backend/user_auth_service/.env.template | Removed outdated template |
| backend/event_ticketing_service/app/services/email.py | Initialized and used APP_BASE_URL for dynamic links and fallbacks |
| backend/event_ticketing_service/.env.template | Removed outdated template |
| .env.template | Updated shared template with stronger secrets and APP_BASE_URL |
Comments suppressed due to low confidence (1)
backend/event_ticketing_service/app/services/email.py:13
- New behavior around
APP_BASE_URLand dynamic link generation isn't covered by existing tests. Please add unit tests to verify fallback logging, URL stripping logic, and correct link interpolation in rendered email templates.
APP_BASE_URL = os.getenv("APP_BASE_URL", "http://localhost:8080")
| return False | ||
| if not APP_BASE_URL: | ||
| logger.error("APP_BASE_URL not set - cannot construct email links") | ||
| # Fallback to a generic domain if not set, but log an error |
Copilot
AI
Jun 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check for missing APP_BASE_URL will never execute because os.getenv('APP_BASE_URL', 'http://localhost:8080') always returns a non-empty string. Consider removing the default in getenv or adjusting this logic to detect an unset environment variable correctly.
| # Fallback to a generic domain if not set, but log an error | |
| # Fallback to a generic domain if not set |
| # Fallback to a generic domain if not set, but log an error | ||
| base_url = "http://resellio.com" | ||
| else: | ||
| base_url = APP_BASE_URL.replace('/api', '') # Ensure we have the root URL |
Copilot
AI
Jun 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Using a simple string replace to remove '/api' may inadvertently strip valid path segments. Consider parsing the URL (e.g., via urllib.parse) to reliably extract and reconstruct the root path.
| base_url = APP_BASE_URL.replace('/api', '') # Ensure we have the root URL | |
| from urllib.parse import urlparse, urlunparse | |
| parsed_url = urlparse(APP_BASE_URL) | |
| path = parsed_url.path.rstrip('/api') if parsed_url.path.endswith('/api') else parsed_url.path | |
| base_url = urlunparse(parsed_url._replace(path=path)) # Ensure we have the root URL |
This pull request introduces updates to environment configurations, email functionality, and infrastructure provisioning to enhance security, flexibility, and maintainability across the Resellio platform. Key changes include refining environment variables, improving email service capabilities, and updating Terraform configurations to handle sensitive data securely and dynamically.
Environment Configuration Updates:
.env.template: Updated database and security-related environment variables for clarity and security, including a strongerSECRET_KEYand newAPP_BASE_URLfor constructing verification links..env.templatefiles frombackend/event_ticketing_serviceandbackend/user_auth_serviceto consolidate configurations. [1] [2]Email Service Enhancements:
backend/event_ticketing_service/app/services/email.py: AddedAPP_BASE_URLfor constructing dynamic links in emails and improved fallback mechanisms for missing configurations. Updated email templates to use dynamic URLs for "View My Tickets" and policy links. [1] [2] [3]Docker Compose Updates:
docker-compose.yml: AddedAPP_BASE_URLas an environment variable for services to ensure consistent link generation across the application. [1] [2]Terraform Configuration Updates:
terraform/main/main.tf: Introduced secrets forsendgrid_api_keyand added new variables foremail_from_addressandapp_base_url. Updated ECS cluster module to dynamically pass these variables to task definitions. [1] [2]terraform/modules/ecs_cluster/main.tf: Updated IAM policy to grant access to the newsendgrid_api_keysecret and modified task definitions to include email-related variables and secrets. [1] [2] [3]terraform/modules/ecs_cluster/variables.tf: Added new variables forsendgrid_api_key_arn,email_from_address, andapp_base_urlto support email functionality.