[OSDEV-899] Move React application to the S3 bucket#535
Conversation
roninzp
had a problem deploying
to
Quality Environment
GitHub Actions
Failure
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
React App | Jest test suite - Code coverage reportTotal: 32.31%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
📝 WalkthroughWalkthroughThe changes reorganize the AWS deployment process to focus on a React application. In the GitHub workflow file, a job has been renamed from Changes
Possibly related PRs
Suggested reviewers
🪧 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: 5
🔭 Outside diff range comments (1)
.github/workflows/deploy_to_aws.yml (1)
141-144: 🛠️ Refactor suggestionUpdate Node.js version.
The workflow is using an outdated version of Node.js (14). Consider updating to a more recent LTS version.
- name: Setup Node.js uses: actions/setup-node@v2 with: - node-version: '14' + node-version: '20'🧰 Tools
🪛 actionlint (1.7.4)
141-141: the runner of "actions/setup-node@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🧹 Nitpick comments (4)
deployment/terraform/lambda-functions/redirect_to_s3_origin/index.mjs (1)
1-2: Remove redundant 'use strict' directive.The 'use strict' directive is unnecessary in ES modules as they are automatically in strict mode.
-'use strict'; -🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
deployment/terraform/cdn.tf (3)
26-32: Enable versioning for better data protection.Versioning is currently disabled. Consider enabling it to protect against accidental deletions and maintain object history.
resource "aws_s3_bucket_versioning" "react" { bucket = aws_s3_bucket.react.id versioning_configuration { - status = "Disabled" + status = "Enabled" } }
139-163: Review default cache behavior TTL settings.The default cache behavior has very aggressive TTL settings (max_ttl = 300s). Consider increasing these values for static assets to improve performance and reduce origin load.
default_cache_behavior { min_ttl = 0 - default_ttl = 0 - max_ttl = 300 + default_ttl = 3600 # 1 hour + max_ttl = 86400 # 24 hours }
666-678: Consider adding retry logic for error responses.The custom error responses for 403 and 404 have a very short caching TTL (10 seconds). Consider increasing this value to reduce load on the origin.
custom_error_response { error_code = 403 - error_caching_min_ttl = 10 + error_caching_min_ttl = 300 response_code = 200 response_page_path = "/index.html" } custom_error_response { error_code = 404 - error_caching_min_ttl = 10 + error_caching_min_ttl = 300 response_code = 200 response_page_path = "/index.html" }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/deploy_to_aws.yml(3 hunks)deployment/terraform/cdn.tf(4 hunks)deployment/terraform/lambda-functions/redirect_to_s3_origin/index.mjs(1 hunks)deployment/terraform/lambda.tf(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
deployment/terraform/lambda-functions/redirect_to_s3_origin/index.mjs
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
🪛 Checkov (3.2.334)
deployment/terraform/lambda.tf
[HIGH] 140-149: Ensure AWS Lambda function is configured to validate code-signing
(CKV_AWS_272)
🪛 actionlint (1.7.4)
.github/workflows/deploy_to_aws.yml
146-146: the runner of "actions/cache@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ 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-fe-code-quality
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: run-countries-code-quality
- GitHub Check: run-django-code-quality
- GitHub Check: run-dd-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: get-base-branch-fe-cov
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: get-base-branch-django-cov
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
had a problem deploying
to
Quality Environment
GitHub Actions
Failure
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
deployment/terraform/lambda.tf (1)
132-146:⚠️ Potential issueLambda Function: Redirect to S3 Origin – Missing Code Signing
The new Lambda function resource is mostly well configured; however, it is missing thecode_signing_config_arnattribute. This attribute is recommended for validating code signatures, as flagged by Checkov (CKV_AWS_272). To address this, please add the following diff:resource "aws_lambda_function" "redirect_to_s3_origin" { filename = "lambda-functions/redirect_to_s3_origin/redirect_to_s3_origin.zip" source_code_hash = filebase64sha256("lambda-functions/redirect_to_s3_origin/redirect_to_s3_origin.zip") function_name = "func${local.short}RedirectToS3origin" role = aws_iam_role.lambda_edge_redirect_to_s3_origin.arn handler = "index.handler" publish = true runtime = "nodejs18.x" provider = aws.certificates + code_signing_config_arn = aws_lambda_code_signing_config.redirect_to_s3_origin.arn depends_on = [ aws_iam_role.lambda_edge_redirect_to_s3_origin, aws_cloudwatch_log_group.redirect_to_s3_origin, ] }Incorporating a valid
aws_lambda_code_signing_configresource reference will enhance security by verifying the Lambda code before deployment.🧰 Tools
🪛 Checkov (3.2.334)
[HIGH] 132-146: Ensure AWS Lambda function is configured to validate code-signing
(CKV_AWS_272)
🧹 Nitpick comments (1)
deployment/terraform/cdn.tf (1)
167-648: Ordered Cache Behaviors Duplication
Numerous ordered cache behaviors are defined for different path patterns. While each is tailored with specific allowed methods, TTL values, and forwarded values, the repeated similar patterns might be consolidated using dynamic blocks or modules to improve readability and maintainability in the long run.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
deployment/terraform/cdn.tf(3 hunks)deployment/terraform/lambda.tf(1 hunks)
🧰 Additional context used
🪛 Checkov (3.2.334)
deployment/terraform/lambda.tf
[HIGH] 132-146: Ensure AWS Lambda function is configured to validate code-signing
(CKV_AWS_272)
⏰ 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-fe-code-quality
- 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-contricleaner-cov
- GitHub Check: get-base-branch-django-cov
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: get-base-branch-countries-cov
🔇 Additional comments (19)
deployment/terraform/cdn.tf (14)
1-3: S3 Bucket Name Construction
The local variable for constructing the S3 bucket name is clear and consistent. Note that using string interpolation withreplaceandlowerfunctions ensures a normalized bucket name.
5-12: S3 Bucket Resource Configuration
Theaws_s3_bucketresource named "react" is set up correctly withforce_destroyenabled and appropriate tags attached based on the constructed name.
14-24: Server-Side Encryption Configuration
The encryption block correctly applies the AES256 algorithm and disables the bucket key. This satisfies common security best practices for S3 buckets.
26-32: Bucket Versioning Settings
Disabling versioning here appears intentional. Ensure that disabling versioning is acceptable for your data retention policy, especially given thatforce_destroyis enabled.
34-40: Ownership Controls Applied
Settingobject_ownershipto "BucketOwnerEnforced" is a good security practice to prevent unwanted access conflicts.
42-81: IAM Policy Document for Bucket Policies
The IAM policy document correctly denies insecure transport and allows CloudFront access. The condition block for "aws:SecureTransport" is precise.
83-87: S3 Bucket Policy Attachment
Associating the bucket policy with the generated IAM policy document ensures that security rules are enforced across the bucket resources.
88-90: CloudFront Origin Access Identity
The configuration for the CloudFront origin access identity is concise. The use of the bucket name as a comment aids in identification.
92-97: Core CloudFront Distribution Settings
The CloudFront distribution resource uses thedepends_onattribute appropriately to ensure that the required buckets (including logs) are provisioned. Thedefault_root_objectis set to "index.html", which fits the serving of a static frontend.
100-115: Origin Configuration: ALB/Custom Origin
The first origin block specifies a custom origin with HTTPS-only policy and includes a custom header for authentication. Everything appears correctly configured.
117-129: Origin Configuration: S3 Bucket
The second origin correctly leverages the bucket’s regional domain name and includes ans3_origin_configwith the appropriate origin access identity. The custom header here ensures consistency with the first origin.
139-165: Default Cache Behavior and Lambda Association
The default cache behavior points to the S3 origin ("originS3") and includes a lambda function association for viewer requests. The TTL settings and protocol policy ensure considerations for both performance and security.
650-654: Logging Configuration
The logging configuration is correctly set up to log to the designated bucket with an appropriate prefix while excluding cookies. This helps with monitoring CDN access without inadvertently logging sensitive data.
662-672: Viewer Certificate and Tags
The viewer certificate block enforces modern TLS settings and the tags appropriately annotate the distribution with project and environment info.deployment/terraform/lambda.tf (5)
95-106: IAM Assume Role Policy for Lambda Function
The policy document permitting the Lambda service principals (lambda.amazonaws.comandedgelambda.amazonaws.com) to assume the role is correctly defined.
108-120: Execution Role Policy for Lambda Function
The execution role policy includes the necessary logging permissions. This is precise and straightforward.
122-125: Lambda Execution Role Creation
The IAM role for the Lambda function uses the assume role policy properly. The naming conventions and usage of local variables ensure consistency.
127-130: IAM Role Policy Attachment for Lambda
Attaching the execution policy to the Lambda role is handled correctly. The resource references are appropriately used.
148-151: CloudWatch Log Group for Lambda Function
The CloudWatch log group is configured with a sensible retention period and naming convention to monitor the Lambda function’s logs.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
deployment/terraform/cdn.tf (1)
165-647: Ordered Cache Behaviors for Specific Endpoint Patterns
Numerous ordered cache behavior blocks are defined for paths such as"tile/*","api/*","/api-auth/*", and various static and user-related routes. They share a consistent structure regarding allowed and cached methods, forwarding settings, TTL values, and compression options.
- The
"tile/*"behavior notably uses a longer default TTL and max TTL, which is appropriate for static tile assets.- Many behaviors forward all headers (using
["*"]), which can impact caching efficiency by enlarging cache keys. Consider whether all headers are necessary or if a subset would suffice.- The TTLs for API and authentication endpoints are set low (default 0; max 300), which aligns with dynamic content, but ensure these values reflect your intended performance and freshness requirements.
- Given the repetition, evaluate if you can leverage Terraform modules or dynamic blocks to reduce duplication and enhance maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/deploy_to_aws.yml(2 hunks)deployment/terraform/cdn.tf(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/deploy_to_aws.yml
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: init-and-plan
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: run-dd-code-quality
- GitHub Check: run-django-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-dd-cov
- GitHub Check: get-base-branch-django-cov
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: run-fe-code-quality
🔇 Additional comments (17)
deployment/terraform/cdn.tf (17)
1-3: Local Variable Definition for Bucket Name
The local block constructs the frontend S3 bucket name by lowercasing and removing spaces from the project name and combining it with the environment and AWS region. This approach is clear and adheres to naming conventions. Be sure that the input variables (e.g.,var.project,var.environment,var.aws_region) are sufficiently validated elsewhere if necessary.
5-12: Definition of the S3 Bucket Resource for React
Theaws_s3_bucketresource for the React frontend is defined with the bucket name set from the local value and withforce_destroyenabled. The tagging is simple and consistent. Note thatforce_destroy = truewill delete all objects when the bucket is removed, so ensure this behavior is intentional for your deployment.
14-24: S3 Bucket Server-Side Encryption Configuration
The encryption settings correctly enforce the use of the AES256 algorithm. The configuration under the rule block is clear. Confirm that the chosen encryption standard satisfies your compliance and security requirements.
26-32: S3 Bucket Versioning Configuration
Versioning is explicitly disabled for the React bucket by setting status to"Disabled". Since this bucket appears dedicated to frontend static assets, this may be acceptable. Just verify that disabling versioning aligns with your data-retention and rollback policies.
34-40: S3 Bucket Ownership Controls
The ownership control is set to"BucketOwnerEnforced", which enhances security by ensuring that the bucket owner maintains full control over object ownership. This is a sound and recommended security practice.
42-81: IAM Policy Document for S3 Access Control
Theaws_iam_policy_documentresource defines two statements:
- The first denies any actions when requests are made over an insecure transport.
- The second allows CloudFront (using its Origin Access Identity) to perform
s3:GetObjectactions.The design is standard for securing S3 buckets accessed via CloudFront. Review the use of wildcard principals in the first statement to ensure it meets your security posture.
83-86: Application of the S3 Bucket Policy
The bucket policy correctly applies the generated JSON from the IAM policy document to the React bucket. This linkage helps enforce the established security rules.
88-90: CloudFront Origin Access Identity Definition
Theaws_cloudfront_origin_access_identityresource is defined with a comment derived from the bucket name which aids in identification. This is a standard and useful practice.
92-97: CloudFront Distribution Dependency Setup
The CloudFront distribution resource declares dependencies on both the logs bucket (aws_s3_bucket.logs) and the React bucket. Ensure that the logs bucket is defined in the deployment (or imported from another module) so that this dependency is valid.
98-115: Primary Origin Block for ALB
The first origin block targets a backend load balancer. It uses a dynamic domain name"origin.${local.domain_name}"and includes a custom header for CloudFront authentication.
- Verify that
local.domain_nameis defined (or provided via variables) in your configuration.- The custom header usage is consistent with enforcing access authentication.
117-129: Secondary Origin Block for React S3 Bucket
This origin block correctly sets the domain name using the S3 bucket's regional domain and applies an S3-origin configuration block with the appropriate Origin Access Identity. The inclusion of the same custom header for authentication ensures consistency across origins.
131-138: Global CloudFront Distribution Settings
The general settings (enabled flag, IPv6, HTTP/2, comment, price class, and aliases) are set appropriately. The alias useslocal.domain_name, so it’s important that this local variable is defined in the scope or passed in as a variable. These settings are aligned with best practices for performance and modern protocols.
[approve]
139-163: Default Cache Behavior for the React Application
The default cache behavior directs requests to the S3 origin (originS3). It restricts allowed methods toGETandHEAD, and forwards query strings and cookies.
- The inclusion of a lambda function association on the
viewer-requestevent (likely for redirection logic) adds dynamic processing; verify that the associated Lambda function is correctly configured and meets performance requirements.- Note that compression is set to
falsehere. Consider whether enabling compression might be beneficial for static asset delivery (unless pre-compressed files are used or the Lambda function relies on uncompressed payloads).
649-653: Logging Configuration for CloudFront Distribution
The logging configuration directs logs to the domain name of the logs bucket with a specific prefix. Make sure theaws_s3_bucket.logsresource exists and that the bucket has proper permissions and retention settings.
655-659: Geo-Restriction Setup
The restrictions block currently setsrestriction_typeto"none", meaning no geo-based access restrictions are applied. This is acceptable if you intend to serve a global audience; otherwise, consider whether any geo-restrictions are necessary.
661-665: Viewer Certificate Configuration
The viewer certificate is configured using an ACM certificate ARN provided by themodule.cert_cdn. Ensure that this module is correctly outputting the expected certificate ARN and that it is valid for your domain.
667-671: Tagging of CloudFront Distribution
The tags applied on the CloudFront distribution enable easy identification by project and environment. This is a good practice for resource organization and auditing.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
doc/release/RELEASE-NOTES.md (1)
25-51: Enhanced Architecture/Environment Section with Endpoint Redirection DetailsThe added section for Release 1.31.0 provides a clear and detailed description of the new deployment strategy: splitting the Django container into two components (FE: React and BE: Django) and redirecting specific endpoints to the Django container. This change aligns with the PR objective to improve ECS resource efficiency and frontend performance.
Points to Consider:
- Endpoint Formatting Consistency: Notice that some endpoints (e.g.,
tile/*on line 28) lack a leading slash, while others (e.g.,/api-auth/*on line 30 and/adminon line 42) include one. Standardizing these (either by including or omitting the leading slash uniformly) could improve readability and reduce potential confusion.- Impact Context: Although the technical changes are well-documented, consider adding a brief note on the expected impact or any necessary action items for clients or downstream consumers. For example, “Ensure that any hardcoded references to these endpoints are reviewed for compatibility with the new redirection logic.”
Overall, the new documentation is comprehensive and clear. Great work on detailing both the rationale and the implementation specifics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
doc/release/RELEASE-NOTES.md(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-fe-code-quality
- 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
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
deployment/terraform/cdn.tf (5)
1-3: Local Variable Definition – Frontend Bucket Name
The locals block defines the bucket name using a combination of string functions. Verify that removing spaces viareplace(var.project, " ", "")meets your naming standards and that all variable values (project, environment, aws_region) are properly provided.
14-24: Server‐Side Encryption Configuration
The encryption rule using"AES256"is configured correctly. It might be useful to document why bucket key management is not enabled (i.e.bucket_key_enabled = false) if that is a conscious security decision.
117-129: Secondary Origin – S3 Bucket
The second origin block configures the S3 origin using the bucket’s regional domain name. The inclusion of a custom header here is notable; if the header value is the same as that passed in the ALB origin block, consider refactoring to avoid duplication.
[verified]
139-163: Default Cache Behavior Configuration
This block directs general traffic to the S3 origin. It includes a lambda function association for viewer-request redirection. Verify the TTL settings (min: 0, default: 0, max: 300) as these will affect dynamic content delivery, and reconsider whether response compression should be enabled.
165-515: Ordered Cache Behaviors for Specific Path Patterns
Multiple ordered cache behavior blocks for paths such as"tile/*","api/*","/api-auth/*","/api-token-auth/*","/api-feature-flags/*","/web/environment.js", and others are defined.
• Please verify that each block’s allowed methods, cached methods, TTL values, and viewer protocol policies accurately reflect the intended caching and delivery strategy.
• Consider parameterizing or modularizing repeated configuration elements (e.g. headers, TTL values) to reduce duplication and make future updates easier.doc/release/RELEASE-NOTES.md (1)
11-19: Database Changes & Migrations Placeholders
The “Database changes” section contains placeholder text (e.g. “Describe high-level database changes.”, “Describe migrations here.”). Before final release, please replace these placeholders with detailed descriptions so all migration and schema changes are fully documented.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
14-14: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
deployment/terraform/cdn.tf(4 hunks)doc/release/RELEASE-NOTES.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
doc/release/RELEASE-NOTES.md
14-14: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
⏰ 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-fe-code-quality
- 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-contricleaner-cov
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: get-base-branch-countries-cov
🔇 Additional comments (14)
deployment/terraform/cdn.tf (11)
5-12: S3 Bucket Resource for React Assets
The S3 bucket resource creates the frontend bucket withforce_destroyenabled. Confirm that automatic deletion of all objects on bucket deletion is intended. Also, ensure that the tag “Name” aligns with your overall tagging strategy.
26-32: Bucket Versioning Resource
Versioning is explicitly disabled. Please double-check that you do not require versioning for your static assets—especially if accidental object deletion might be an issue in the future.
34-40: Bucket Ownership Controls
The ownership controls are set to enforceBucketOwnerEnforced. This is a good practice for ensuring that the bucket’s objects remain under the control of the owner.
42-81: IAM Policy Document for S3 Bucket Access
The policy denies insecure transport and allows access to CloudFront through the designated origin access identity. Verify that the condition (checking"aws:SecureTransport" == "false") works as expected and that the principal identifier fromaws_cloudfront_origin_access_identity.react.iam_arnis correct.
83-86: S3 Bucket Policy Resource
This resource attaches the IAM policy defined above to the S3 bucket. The linkage is clear and appears correct.
88-90: CloudFront Origin Access Identity
The CloudFront OAI resource is defined with a descriptive comment. Ensure that this identity is referenced consistently in your distribution configuration.
100-115: Primary Origin – ALB Configuration
The first origin block defines an origin with a custom origin configuration (likely for the backend load balancer). Confirm that the domain name pattern ("origin.${local.domain_name}") and the associated custom header for"X-CloudFront-Auth"are correct and secure.
649-654: Logging Configuration for CloudFront Distribution
The logging configuration points to an S3 bucket for logs. Double-check that this aligns with your retention and auditing policies.
655-659: Restrictions Block
The geo restriction is set to “none”. Confirm that no country-level restrictions are needed for your application.
[verified]
661-666: Viewer Certificate Configuration
The viewer certificate section properly references an ACM certificate from your certificate module. Ensure that the minimum protocol version ("TLSv1.2_2018") satisfies your security requirements.
667-671: Resource Tagging for the Distribution
Tags are assigned forProjectandEnvironment, which supports resource tracking. Verify that these tags are consistent with your overall tagging strategy.doc/release/RELEASE-NOTES.md (3)
6-10: Release Version Section – “Release 2.0.0”
A new release section for version 2.0.0 is introduced. This major version update aligns with the architectural changes described in this PR. Ensure that the version number is updated consistently everywhere and that the introductory text clearly explains the separation of frontend (React) and backend (Django) components.
20-22: Code/API Changes Section
A new “Code/API changes” section is added. Confirm that this section captures all modifications in the codebase that affect the API, and update with precise details as needed.
23-51: Architecture/Environment Changes Documentation
This section details the restructuring of the application deployment, especially the split of the Django container into separate FE (React) and BE (Django) components along with the redirection of endpoints via CloudFront. The list of endpoints to be redirected is comprehensive.
• Double-check that all endpoint paths and descriptions match the live configuration.
• Consider adding a brief explanation of the benefits of this new architecture for future readers.
VadimKovalenkoSNF
left a comment
VadimKovalenkoSNF
left a comment
There was a problem hiding this comment.
Minor change in release notes requested.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
doc/release/RELEASE-NOTES.md (4)
15-15: Remove Trailing Punctuation from Headings
The heading "#### Migrations:" contains a trailing punctuation mark. To conform with the Markdown style guidelines (MD026), consider removing the colon (e.g. change to "#### Migrations").-#### Migrations: +#### Migrations🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
15-15: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
6-14: New Release 2.0.0 Section – Verify Placeholders and Consistency
The new section for Release 2.0.0 has been added (lines 6–14). Please ensure that placeholder texts (e.g. in "### Database changes", "#### Migrations", "#### Schema changes") are eventually replaced with final content before release. Also, verify that the version number and release date (“March 22, 2025”) are correct and align with your deployment plans.If you need help drafting the detailed change descriptions for each subsection, let me know.
24-52: Endpoint Redirection Documentation – Formatting and Clarity
The "### Architecture/Environment changes" block (lines 24–52) for Release 2.0.0 lists all endpoints that will be redirected to the Django container. The list is comprehensive; however, consider:• Using a consistent indentation or code formatting (e.g. a code block) for the endpoints list to improve readability.
• Double-check that each endpoint path is correct and that there are no accidental duplicates or typos.This additional clarity will help both developers and operations teams verify the new routing rules.
53-60: Typo in “What's New” Section
In the “### What's new” section (lines 53–60) the sentence “... additional data won't be send to the server ...” contains a minor grammatical error. Please change “send” to “sent” to ensure clarity and correctness.- additional data won't be send to the server + additional data won't be sent to the server
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
doc/release/RELEASE-NOTES.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
doc/release/RELEASE-NOTES.md
15-15: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: run-dd-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: run-fe-code-quality
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: run-django-code-quality
- GitHub Check: get-base-branch-fe-cov
- GitHub Check: run-countries-code-quality
- GitHub Check: get-base-branch-django-cov
🔇 Additional comments (1)
doc/release/RELEASE-NOTES.md (1)
61-65: Release Instructions Verification for Release 2.0.0
The “### Release instructions:” block (lines 61–65) correctly reminds users to include themigrateandreindex_databasecommands. Please verify that these instructions are comprehensive and that any environment-specific post-deployment actions (if any) are adequately documented.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
61-61: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
63-63: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
64-64: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
doc/release/RELEASE-NOTES.md (3)
15-17: Remove Trailing Punctuation from Heading
The heading “#### Migrations:” includes a trailing colon. Removing the colon will adhere to markdown style guidelines (see MD026, no-trailing-punctuation).-#### Migrations: +#### Migrations🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
15-15: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
24-27: Improve Wording for Container Restructuring
Within the Architecture/Environment changes section, the bullet for [OSDEV-899] uses “Splitted the Django container…” Consider replacing “Splitted” with “Split” for grammatical correctness.-* [OSDEV-899](https://opensupplyhub.atlassian.net/browse/OSDEV-899) - Splitted the Django container into two components: FE (React) and BE (Django)... +* [OSDEV-899](https://opensupplyhub.atlassian.net/browse/OSDEV-899) - Split the Django container into two components: FE (React) and BE (Django)...
61-65: Remove Trailing Punctuation from Heading
The “### Release instructions:” heading ends with a colon. Removing the colon will bring the heading in line with best practices.-### Release instructions: +### Release instructions🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
61-61: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
63-63: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
64-64: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
doc/release/RELEASE-NOTES.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
doc/release/RELEASE-NOTES.md
15-15: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
⏰ 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-dd-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: run-fe-code-quality
- GitHub Check: run-django-code-quality
- GitHub Check: run-countries-code-quality
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: get-base-branch-fe-cov
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: get-base-branch-django-cov
🔇 Additional comments (1)
doc/release/RELEASE-NOTES.md (1)
6-7: Release Version Header Updated
The new release header “## Release 2.0.0” correctly reflects the major update and meets the PR objectives.
3 participants
With this task, we split the Django container into two components: FE (React) and BE (Django). Requests to the frontend (React) will be processed by the CDN (CloudFront), while requests to the API will be redirected to the Django container. This approach will allow for more efficient use of ECS cluster computing resources and improve frontend performance.
The following endpoints will be redirected to the Django container:
tile/*
api/*
/api-auth/*
/api-token-auth/*
/api-feature-flags/*
/web/environment.js
/admin/*
/health-check/*
/rest-auth/*
/user-login/*
/user-logout/*
/user-signup/*
/user-profile/*
/user-api-info/*
/admin
/static/admin/*
/static/django_extensions/*
/static/drf-yasg/*
/static/gis/*
/static/rest_framework/*
/static/static/*
/static/staticfiles.json
All other traffic will be redirected to the React application.