Aws#392
Open
gmorales96 wants to merge 9 commits intomasterfrom
Open
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR centralizes environment configuration management through a new arcusd.config module that loads variables from environment or AWS Secrets Manager, refactors all service modules to consume configuration constants instead of reading Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (1)
infrastructure/lib/platform-stack.ts (1)
24-27: ⚡ Quick win
SecretValue.unsafePlainTexttriggers a CDK synthesis Nag warning.Using
unsafePlainText(even with'{}') causes CDK to emit[Error at /...] Resolution error: Unsafe use of 'unsafePlainText'. For a placeholder secret that will be populated externally, omitsecretStringValueentirely — AWS Secrets Manager will create the secret without an initial value, which can then be set via the console or CLI.♻️ Proposed fix
const secretsConfig = new secretsmanager.Secret(this, 'secrets-config', { secretName: `arcusd-${stage}-secrets-config`, - secretStringValue: SecretValue.unsafePlainText('{}'), });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infrastructure/lib/platform-stack.ts` around lines 24 - 27, The Secret is created using secretsmanager.Secret (variable secretsConfig) with secretStringValue: SecretValue.unsafePlainText('{}'), which triggers a CDK synthesis Nag warning; remove the secretStringValue property from the secretsmanager.Secret constructor so the secret is created without an initial value (allowing it to be populated externally) — locate the secretsConfig instantiation in PlatformStack (the secretsmanager.Secret(...) call) and delete the secretStringValue: SecretValue.unsafePlainText('{}') entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@arcusd/arcusactions.py`:
- Around line 8-13: The import in arcusactions.py currently uses an absolute
import from arcusd.config; change it to a relative import so the module loads
from the same package. Replace the absolute import statement that brings in
ARCUS_API_KEY, ARCUS_SECRET_KEY, ARCUS_USE_SANDBOX, and TOPUP_BILLERS from
arcusd.config with a relative import (e.g., use .config) so the symbols
ARCUS_API_KEY, ARCUS_SECRET_KEY, ARCUS_USE_SANDBOX, and TOPUP_BILLERS are
imported from the local package module.
In `@arcusd/config.py`:
- Around line 30-43: The boolean parsing for ARCUS_USE_SANDBOX and ARCUSD_PROD
is too strict (only 'true') and ARCUSD_WORKERS is left as a string; normalize
and robustly parse these env vars: read via os.environ.get(..., '') then
.strip().lower() and treat values like '1','true','yes','on','y' as True
(everything else False) for ARCUS_USE_SANDBOX and ARCUSD_PROD, and convert
ARCUSD_WORKERS to an int (falling back to 0) so ARCUSD_WORKERS is an integer;
update the assignments to use these normalized checks referencing the
ARCUS_USE_SANDBOX, ARCUSD_PROD, and ARCUSD_WORKERS symbols.
- Around line 19-20: The loop that sets environment variables directly from
config (for key, value in config.items(): os.environ[key] = value) can crash if
a value is not a string; update the code that writes to os.environ (the for loop
in arcusd/config.py that iterates config.items()) to normalize values by
converting them to strings (e.g., use str(value) or an explicit check and cast)
before assignment so non-string secrets won't raise a TypeError when setting
os.environ.
In `@arcusd/daemon/__init__.py`:
- Line 4: Replace the absolute import of the internal config module with a
relative import: change the import statement that currently references
arcusd.config to use the parent-package relative form and import ARCUSD_PROD and
ARCUSD_SENTRY_DSN from ..config so the __init__.py module uses a relative import
for the internal config module.
In `@arcusd/daemon/celeryconfig.py`:
- Line 1: Change the absolute import in celeryconfig.py to a relative import:
replace the current statement that imports ARCUSD_AMPQ_ADDRESS and
ARCUSD_PAYMENTS_QUEUE from arcusd.config with a relative import from the parent
package (use ..config and import the symbols ARCUSD_AMPQ_ADDRESS and
ARCUSD_PAYMENTS_QUEUE) so the module follows the package-relative import
guideline.
In `@arcusd/data_access/providers_mapping.py`:
- Around line 3-4: Replace the absolute imports with package-relative imports:
change the current "from arcusd.config import ARCUSD_DB_MONGO_URL" and "from
arcusd.exc import UnknownServiceProvider" to use relative imports from the
parent package (i.e., import ARCUSD_DB_MONGO_URL and UnknownServiceProvider via
..config and ..exc respectively) so providers_mapping.py uses internal relative
imports.
- Around line 10-15: The lazy Mongo initialization in _get_db can race and
create multiple MongoClient instances; fix by adding a module-level lock (e.g.,
_db_init_lock = threading.Lock()) and use it to protect the first-time
initialization: perform a quick check of _db, acquire _db_init_lock, then
re-check _db and only then assign _client = MongoClient(ARCUSD_DB_MONGO_URL) and
_db = _client.get_database() (double-checked locking pattern) so concurrent
calls to _get_db do not create multiple clients.
In `@arcusd/data_access/tasks.py`:
- Line 3: Replace the absolute import of the internal config module with a
relative import: instead of importing ARCUSD_DB_MONGO_URL from arcusd.config,
import it from the package-local config module so the symbol ARCUSD_DB_MONGO_URL
is sourced via a relative import (update the import statement at the top of
tasks.py that currently references arcusd.config to use the package-local config
import). Ensure the module name remains config and no other references change.
In `@infrastructure/bin/infrastructure.ts`:
- Around line 10-13: The stack env currently hardcodes region/account causing
secret ARNs and cdk.context leakage; replace the literal env values with
process.env.CDK_DEFAULT_ACCOUNT and process.env.CDK_DEFAULT_REGION for the stack
env object, and remove the embedded account from the Secrets Manager ARN by
reading the secret ARN (or account) from an environment variable or SSM
parameter instead of hardcoding; update any code that constructs or passes the
Secrets Manager ARN to use the injected env var/SSM value and ensure stacks are
instantiated using the new env object (CDK_DEFAULT_ACCOUNT/CDK_DEFAULT_REGION).
In `@infrastructure/lib/arcusd-stack.ts`:
- Around line 36-69: The executionRole (executionRole) lacks ECR pull
permissions and is incorrectly reused as the taskRole on workerTaskDef; create a
distinct task role and give the executionRole the managed ECS execution policy
plus ECR pull rights. Concretely: attach the managed policy
AmazonECSTaskExecutionRolePolicy to executionRole, grant it ECR pull access
(either call repository.grantPull(executionRole) for the repository created via
ecr.Repository.fromRepositoryName or add an inline policy with
ecr:GetAuthorizationToken, ecr:BatchGetImage, ecr:GetDownloadUrlForLayer), and
then create a separate iam.Role (e.g., taskRole) for the container runtime and
pass that as taskRole to the ecs.FargateTaskDefinition instead of reusing
executionRole.
In `@infrastructure/package.json`:
- Around line 4-6: The package.json "bin" entry currently maps the
"infrastructure" npm binary to "bin/core.js" which does not exist after tsc
emits "bin/infrastructure.js" (the TS entrypoint is bin/infrastructure.ts);
update the package.json "bin" value for the "infrastructure" key to point to the
actual compiled file (bin/infrastructure.js) or adjust your build/tsconfig (add
an outDir and/or a postbuild step) so that bin/core.js is produced, ensuring the
mapping between the "infrastructure" bin entry and the compiled output matches
the TS entrypoint name.
In `@requirements.txt`:
- Around line 3-7: The requirements pin for the Sentry package currently reads
"sentry-sdk==0.16.0" which is vulnerable; update the dependency to a safe
released version by replacing that exact pin with a constraint that requires at
least 1.45.1 (for example "sentry-sdk>=1.45.1,<2.0.0" or "sentry-sdk==1.45.1")
so the project installs a non-vulnerable Sentry SDK; ensure the modified
requirement string replaces the existing "sentry-sdk==0.16.0" entry in
requirements.txt and run your dependency install/tests to verify compatibility.
---
Nitpick comments:
In `@infrastructure/lib/platform-stack.ts`:
- Around line 24-27: The Secret is created using secretsmanager.Secret (variable
secretsConfig) with secretStringValue: SecretValue.unsafePlainText('{}'), which
triggers a CDK synthesis Nag warning; remove the secretStringValue property from
the secretsmanager.Secret constructor so the secret is created without an
initial value (allowing it to be populated externally) — locate the
secretsConfig instantiation in PlatformStack (the secretsmanager.Secret(...)
call) and delete the secretStringValue: SecretValue.unsafePlainText('{}') entry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fa738ece-44bf-434a-b88f-35fe761547b8
⛔ Files ignored due to path filters (1)
infrastructure/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (20)
.gitignorearcusd/arcusactions.pyarcusd/callbacks.pyarcusd/config.pyarcusd/daemon/__init__.pyarcusd/daemon/celeryconfig.pyarcusd/data_access/providers_mapping.pyarcusd/data_access/tasks.pyinfrastructure/.gitignoreinfrastructure/.npmrcinfrastructure/bin/infrastructure.tsinfrastructure/cdk.context.jsoninfrastructure/cdk.jsoninfrastructure/lib/arcusd-stack.tsinfrastructure/lib/platform-stack.tsinfrastructure/package.jsoninfrastructure/tsconfig.jsonrequirements-dev.txtrequirements.txttests/test_config.py
💤 Files with no reviewable changes (1)
- .gitignore
Comment on lines
+4
to
+6
| "bin": { | ||
| "infrastructure": "bin/core.js" | ||
| }, |
There was a problem hiding this comment.
bin.infrastructure points to a non-existent compiled file.
bin/core.js is referenced in the bin field, but the TypeScript entrypoint is bin/infrastructure.ts. Without an outDir in tsconfig.json, tsc emits bin/infrastructure.js — not bin/core.js. The npm binary would be broken after a tsc build.
🐛 Proposed fix
"bin": {
- "infrastructure": "bin/core.js"
+ "infrastructure": "bin/infrastructure.js"
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "bin": { | |
| "infrastructure": "bin/core.js" | |
| }, | |
| "bin": { | |
| "infrastructure": "bin/infrastructure.js" | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@infrastructure/package.json` around lines 4 - 6, The package.json "bin" entry
currently maps the "infrastructure" npm binary to "bin/core.js" which does not
exist after tsc emits "bin/infrastructure.js" (the TS entrypoint is
bin/infrastructure.ts); update the package.json "bin" value for the
"infrastructure" key to point to the actual compiled file
(bin/infrastructure.js) or adjust your build/tsconfig (add an outDir and/or a
postbuild step) so that bin/core.js is produced, ensuring the mapping between
the "infrastructure" bin entry and the compiled output matches the TS entrypoint
name.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Makefile (1)
2-2: ⚡ Quick winConsider centralizing the compose command to avoid drift.
You already centralize
runviaDOCKER; doing the same forbuild/stop/rm/downkeeps future CLI changes in one place.Suggested refactor
-DOCKER=docker compose run --rm arcusd +DOCKER_COMPOSE=docker compose +DOCKER=$(DOCKER_COMPOSE) run --rm arcusd @@ - docker compose build + $(DOCKER_COMPOSE) build @@ - docker compose stop - docker compose rm -f + $(DOCKER_COMPOSE) stop + $(DOCKER_COMPOSE) rm -f @@ - docker compose down --rmi local + $(DOCKER_COMPOSE) down --rmi localAlso applies to: 34-34, 49-50, 60-60
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` at line 2, The Makefile duplicates the docker-compose CLI across targets; introduce a single compose command variable (reuse or extend the existing DOCKER variable into a COMPOSE_CMD/DOCKER_COMPOSE variable) and update all targets that currently call docker compose (the targets invoking build, stop, rm, down and the existing run usage) to reference that variable instead of hardcoding the command so future CLI changes are centralized (update usages in targets named build, stop, rm, down and the current DOCKER run reference).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Makefile`:
- Line 2: The Makefile duplicates the docker-compose CLI across targets;
introduce a single compose command variable (reuse or extend the existing DOCKER
variable into a COMPOSE_CMD/DOCKER_COMPOSE variable) and update all targets that
currently call docker compose (the targets invoking build, stop, rm, down and
the existing run usage) to reference that variable instead of hardcoding the
command so future CLI changes are centralized (update usages in targets named
build, stop, rm, down and the current DOCKER run reference).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b7c2c176-a936-4a45-a235-8b2dd4af633f
📒 Files selected for processing (6)
Makefilearcusd/arcusactions.pyarcusd/daemon/__init__.pyarcusd/daemon/celeryconfig.pyarcusd/data_access/providers_mapping.pyarcusd/data_access/tasks.py
✅ Files skipped from review due to trivial changes (5)
- arcusd/data_access/tasks.py
- arcusd/daemon/init.py
- arcusd/daemon/celeryconfig.py
- arcusd/arcusactions.py
- arcusd/data_access/providers_mapping.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #392 +/- ##
==========================================
+ Coverage 98.51% 98.66% +0.14%
==========================================
Files 35 37 +2
Lines 743 824 +81
==========================================
+ Hits 732 813 +81
Misses 11 11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Infrastructure
Configuration Management
Chores
Tests