[OSDEV-1947] Introduced support for RBA deployment, disabled CSV export in RBA, and fixed FE environment check.#614
Conversation
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
had a problem deploying
to
Quality Environment
GitHub Actions
Failure
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
React App | Jest test suite - Code coverage reportTotal: 34.23%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Dedupe Hub App | Unittest test suite - Code coverage reportTotal: 56.14%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Countries App | Unittest test suite - Code coverage reportTotal: 100%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Contricleaner App | Unittest test suite - Code coverage reportTotal: 98.75%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
Django App | Unittest test suite - Code coverage reportTotal: 81.06%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
📝 Walkthrough""" WalkthroughSupport for a new "RBA" environment was added across deployment workflows, infrastructure configuration, and application settings. This includes workflow logic updates, new Terraform variables, ECS environment mapping, and documentation. Additionally, environment checks in the frontend were updated to use "local" instead of "development" for local development, and a toggle for the CSV export scheduler was introduced. Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant GitHub Actions
participant Terraform
participant Django App
participant React App
Developer->>GitHub Actions: Push branch/tag (e.g., rba-*)
GitHub Actions->>GitHub Actions: Detect branch prefix or deploy-env input
GitHub Actions->>GitHub Actions: Set environment to "RBA"
GitHub Actions->>Terraform: Apply with terraform-rba.tfvars
Terraform->>AWS: Deploy infrastructure for RBA
GitHub Actions->>Django App: Deploy with RBA settings
GitHub Actions->>React App: Deploy with ENVIRONMENT="rba"
React App->>React App: Initialize Rollbar/GA (skip if ENVIRONMENT="local")
Suggested reviewers
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (13)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 1
🔭 Outside diff range comments (1)
.github/workflows/deploy_to_aws.yml (1)
4-8: 💡 Verification agent🧩 Analysis chain
Verify triggers for RBA branches
While you've added RBA mapping in job environment resolution, the
on.push.branchessection currently only includesmainandreleases/**. If you intend to automatically run this workflow on branches prefixed withrba-, you should add a matching pattern (e.g.,rba-*). Otherwise, pushes to RBA branches will never trigger the workflow.
🏁 Script executed:
#!/bin/bash # Check if 'rba-' branch patterns are included in push triggers rg -n "push:" -A5 .github/workflows/deploy_to_aws.ymlLength of output: 2575
Include RBA branch triggers
The
on.push.branchessection only coversmainandreleases/**. Pushes to branches prefixed withrba-won’t trigger this workflow. To enable automatic runs on those branches, update the trigger as follows:on: push: branches: - main - 'releases/**' + - 'rba-*'File: .github/workflows/deploy_to_aws.yml Lines 4–8
♻️ Duplicate comments (10)
.github/workflows/deploy_to_aws.yml (10)
91-91: Duplicate: RBA environment resolution logic
129-129: Duplicate: RBA environment resolution logic
195-195: Duplicate: RBA environment resolution logic
289-289: Duplicate: RBA environment resolution logic
311-311: Duplicate: RBA environment resolution logic
335-335: Duplicate: RBA environment resolution logic
381-381: Duplicate: RBA environment resolution logic
407-407: Duplicate: RBA environment resolution logic
428-428: Duplicate: RBA environment resolution logic
488-488: Duplicate: RBA environment resolution logic
🧹 Nitpick comments (12)
.github/workflows/deploy_to_aws.yml (1)
42-42: Extend job environment resolution to include RBAYou’ve correctly updated the
environmentexpression to handle branches starting withrba-. Consider refactoring to DRY this logic, for example by defining a top-levelenvvariable or YAML anchor and reusing it across jobs.+env: + RESOLVED_ENV: ${{ inputs.deploy-env || + (github.ref_name == 'main' && 'Development') || + (startsWith(github.ref_name, 'releases/') && 'Pre-prod') || + (startsWith(github.ref_name, 'production-') && 'Production') || + (startsWith(github.ref_name, 'sandbox-') && 'Staging') || + (startsWith(github.ref_name, 'rba-') && 'RBA') || + 'None' + }} jobs: init-and-plan: - environment: ${{ inputs.deploy-env || (github.ref_name == 'main' && 'Development') || ... }} + environment: ${{ env.RESOLVED_ENV }}deployment/environments/terraform-rba.tfvars (11)
1-3: Inconsistent environment casingThe
environmentfield is set to"Rba", but other configurations (e.g., GitHub workflows) refer toRBA. For consistency, consider using the same uppercase notation:-environment = "Rba" +environment = "RBA"
4-9: Review AWS region and DNS configuration
- RBA uses two AZs (
eu-west-1a,eu-west-1b); if your network and budget allow, adding a third AZ improves resilience.- Verify the private (
osh.internal) and public (opensupplyhub.org) hosted zones are correct for RBA.
10-11: Optimize CloudFront cost for non-production
cloudfront_price_class = "PriceClass_All"covers all edge locations at higher cost. For RBA, considerPriceClass_100orPriceClass_200based on traffic needs.
15-23: Ensure RDS variable types align with Terraform schemaValues like
rds_allocated_storage = "256"andrds_engine_version = "16"are strings. Checkdeployment/terraform/variables.tfto confirm these are declared asstring; otherwise remove quotes. Also, sincerds_multi_az = false, review if multi-AZ should be enabled for production-like reliability.
24-30: Right-size ECS tasks for RBAThe values (
app_ecs_desired_count = "4",app_fargate_cpu = "1024", etc.) mirror production defaults. If RBA is lower-traffic, consider reducing replicas or resource allocations to save cost.
31-37: Review ECS auto-scaling thresholdsCPU thresholds (
85%/15%) and capacities (min_capacity = 2,max_capacity = 4) may be over-provisioned. Tailor these to RBA workload patterns.
38-43: Validate CLI and Batch compute sizing
cli_fargate_memory = "8192"may be overkill for non-critical tasks.- Confirm
batch_default_ce_spot_fleet_bid_percentage = 60andbatch_ami_idare correct for RBA workloads.
44-49: Optimize Batch job resource limitsThe
batch_default_ce_max_vcpus = 128and instance types["c5", "m5"]are broad. If RBA jobs are smaller, restrict vCPU limits and instance types to control costs.
50-53: Verify caching and timeout settings
- Check
ec_memcached_identifier = "opensupplyhub-rba"aligns with existing naming conventions.- Ensure
gunicorn_worker_timeout = "360"is appropriate for RBA application performance.
60-64: Fix formatting ondjango_log_levelMissing spaces around
=reduces readability. Apply:-django_log_level="DEBUG" +django_log_level = "DEBUG"Also verify
app_cc_ecs_desired_countandapp_ddsettings fit RBA load.
66-69: Review OpenSearch and Logstash sizing
opensearch_instance_type = "m6g.large.search"may exceed RBA requirements.- Check
app_logstash_fargate_cpu = 256andapp_logstash_fargate_memory = 2048are sufficient for log volumes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/deploy_to_aws.yml(13 hunks).github/workflows/release-deploy.yml(2 hunks)deployment/environments/terraform-rba.tfvars(1 hunks)deployment/terraform/batch.tf(1 hunks)deployment/terraform/variables.tf(1 hunks)doc/release/RELEASE-NOTES.md(1 hunks)src/django/oar/settings.py(3 hunks)src/django/web/views.py(1 hunks)src/react/public/index.html(1 hunks)src/react/src/util/util.ga.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: run-django-code-quality
- GitHub Check: run-dd-code-quality
- GitHub Check: run-countries-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: get-base-branch-fe-cov
- GitHub Check: get-base-branch-django-cov
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: run-fe-code-quality
🔇 Additional comments (16)
deployment/terraform/variables.tf (1)
684-688: Good addition of the CSV export toggle variable.The new
export_csv_enabledvariable provides a clean way to conditionally enable/disable the CSV export functionality across different environments, which aligns perfectly with the PR's objective to disable this feature in the RBA environment.src/django/web/views.py (1)
21-22: Documentation update correctly reflects all environments.The expanded environment list now properly includes all possible values, including the new
rbaenvironment and the renamedlocalenvironment (previously known asdevelopment).src/react/public/index.html (1)
81-81: Correct fix for environment check.This change fixes the bug mentioned in the PR objectives by correctly checking for the
localenvironment instead ofdevelopment, ensuring Rollbar is initialized properly across all environments.deployment/terraform/batch.tf (1)
287-287: Successfully implemented conditional CSV export scheduling.Using the new
export_csv_enabledvariable to control the CloudWatch event rule'sis_enabledattribute is a clean implementation that allows disabling the CSV export job in the RBA environment without affecting other environments.src/react/src/util/util.ga.js (1)
37-37: Fixed environment check for Google Analytics.This change correctly updates the environment condition from
developmenttolocal, ensuring that Google Analytics tracking is properly handled across all environments, which aligns with the environment naming changes mentioned in the PR..github/workflows/release-deploy.yml (2)
17-17: New deployment environment added successfully.The 'rba' option has been properly added to the deployment environment choices, making it available as a deployment target alongside existing environments.
49-51: Environment variable assignment for RBA deployment looks good.The conditional logic correctly sets ENV="RBA" when the deploy-env input is "rba", following the same pattern used for other environments.
doc/release/RELEASE-NOTES.md (2)
28-30: Clear documentation of RBA environment implementation.The release notes effectively communicate the addition of the RBA environment to the CD pipelines and the introduction of the new infrastructure switch to disable the CSV export job in the RBA environment.
33-33: Frontend environment check bugfix clearly documented.The note clearly explains how the environment check was fixed to use 'local' instead of 'development', which aligns with the actual environment naming in the backend.
src/django/oar/settings.py (3)
37-37: RBA environment correctly added to valid environments.The 'Rba' environment has been properly added to the VALID_ENVIRONMENTS tuple, allowing it to pass environment validation.
430-447: RBA ECS environment configuration implemented correctly.The configuration for the RBA environment in ECSMANAGE_ENVIRONMENTS follows the standard pattern used for other environments, with appropriate naming conventions for AWS resources.
534-538: Improved handling of Hubspot environment variables.The code now explicitly sets Hubspot-related variables to None when their environment variables are unset or empty, which is more explicit and safer than the previous implementation that used the return value of os.getenv() directly.
.github/workflows/deploy_to_aws.yml (1)
21-21: Approve: Added "RBA" to deploy-env choicesGreat addition of the
RBAoption to the workflow_dispatch inputs, enabling explicit RBA deployments.deployment/environments/terraform-rba.tfvars (3)
12-14: Validate Bastion host settings
- Ensure AMI
ami-0bb3fad3c0286ebd5is up-to-date and exists ineu-west-1.- Confirm that
t3.nanoprovides adequate performance, or chooset3.micro/t3.smallas needed.
54-59: Review deduplication and snapshot naming
- Confirm
rds_final_snapshot_identifier = "opensupplyhub-rds-rba"follows your snapshot naming conventions.- Ensure
topic_dedup_basic_name,dedupe_hub_name, anddedupe_hub_versioncorrectly isolate RBA data streams.
71-71:Details
✅ Verification successful
Confirm CSV export toggle usage
You’ve set
export_csv_enabled = falseto disable CSV exports in RBA. Verify this flag is referenced indeployment/terraform/batch.tf(or relevant modules) to conditionally disable the EventBridge scheduler:
🏁 Script executed:
#!/bin/bash # Confirm usage of export_csv_enabled in Terraform rg -n "export_csv_enabled" -A3 deployment/terraform/batch.tfLength of output: 180
export_csv_enabled flag usage confirmed
The
export_csv_enabledvariable is referenced indeployment/terraform/batch.tfon line 287:resource "aws_cloudwatch_event_target" "export_csv" { … is_enabled = var.export_csv_enabled }With
export_csv_enabled = falsein your RBA tfvars, the EventBridge CSV‐export target will be disabled as intended.
VadimKovalenkoSNF
left a comment
VadimKovalenkoSNF
left a comment
There was a problem hiding this comment.
Approved, good work!
|
5 participants
OSDEV-1947
export_csv_enabledinfrastructure switch to disable the Amazon EventBridge scheduler for the CSV export job in the RBA environment, as exporting the database to CSV is not needed in that environment.development. However, the environment had been renamed tolocalsome time ago and was no longer passed asdevelopmentfrom the BE during local development