Update "infrastructure" command logic/add various fixes#761
Merged
forstmeier merged 4 commits intomasterfrom Feb 20, 2026
Merged
Update "infrastructure" command logic/add various fixes#761forstmeier merged 4 commits intomasterfrom
forstmeier merged 4 commits intomasterfrom
Conversation
forstmeier
added
python
Contributor
📝 WalkthroughWalkthroughRenames S3 bucket env/config keys to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Contributor
Greptile SummaryThis PR standardizes environment variable naming by adding
Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| applications/equitypricemodel/src/equitypricemodel/server.py | Updated environment variable from AWS_S3_MODEL_ARTIFACTS_BUCKET to AWS_S3_MODEL_ARTIFACTS_BUCKET_NAME for consistency |
| infrastructure/Pulumi.production.yaml | Added YAML document separator (---) at the beginning of the file |
| infrastructure/main.py | Removed protect=True from IAM resources and renamed Pulumi exports to include _name suffix for S3 buckets |
| maskfile.md | Updated environment variables, improved Pulumi organization handling, removed bootstrap flag dependency for imports, and standardized date formatting for datamanager sync |
| tools/prepare_training_data.py | Updated environment variables to use _NAME suffix for consistency |
| tools/sync_equity_categories.py | Updated environment variable to AWS_S3_DATA_BUCKET_NAME and removed unnecessary code comments |
Last reviewed commit: 7bd67ed
Contributor
There was a problem hiding this comment.
Pull request overview
This PR standardizes environment variable naming, updates infrastructure management commands for improved reliability, and removes overly restrictive IAM resource protection. The changes ensure consistent configuration across local development and GitHub Actions workflows.
Changes:
- Renamed S3 bucket environment variables to include
_NAMEsuffix for consistency - Removed
protect=Trueflag from IAM resources while retainingretain_on_delete=Trueto allow Pulumi-managed updates - Moved Pulumi resource imports outside bootstrap conditional to run on every stack update
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tools/sync_equity_categories.py | Updated environment variable name to AWS_S3_DATA_BUCKET_NAME and removed redundant comments |
| tools/prepare_training_data.py | Updated environment variables to AWS_S3_DATA_BUCKET_NAME and AWS_S3_MODEL_ARTIFACTS_BUCKET_NAME |
| maskfile.md | Updated stack up command to import resources on every run; improved datamanager invoke flow to call sync_equity_categories; standardized curl flags; added AWS_S3_DATA_BUCKET_NAME secret |
| infrastructure/main.py | Removed protect=True from IAM policies and roles; updated Pulumi exports and ECS container environment variables to use _NAME suffix |
| infrastructure/Pulumi.production.yaml | Added YAML document separator for proper formatting |
| applications/equitypricemodel/src/equitypricemodel/server.py | Updated environment variable to AWS_S3_MODEL_ARTIFACTS_BUCKET_NAME |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@infrastructure/__main__.py`:
- Line 1167: The ResourceOptions usage
pulumi.ResourceOptions(retain_on_delete=True) can cause Pulumi state drift on
replacement if protect=True is not set; update the code where
pulumi.ResourceOptions(retain_on_delete=True) is used to add a short inline
comment describing the intended behavior and failure mode (resource retained in
AWS but removed from state on replacement, requiring manual pulumi import), and
either add protect=True when you want to prevent replacements or explicitly
document why protect is intentionally omitted for these resources (i.e., keep
pulumi.ResourceOptions(retain_on_delete=True) but add the explanatory comment
next to that expression and replicate the same comment for the other matching
occurrences).
In `@maskfile.md`:
- Around line 198-214: The import commands currently suppress all errors via
"2>/dev/null || true" which hides real failures; update the import logic around
the pulumi import invocations (including the github_actions_infrastructure_role,
github_actions_infrastructure_policy, sagemaker_execution_role,
sagemaker_execution_policy and the GITHUB_POLICY_ARN / SAGEMAKER_POLICY_ARN
lookups) to capture the pulumi import exit status and stderr, only ignore or
treat as success the specific idempotent "already exists/in state" messages, but
on other non-idempotent failures emit a warning or error message (log the stderr
and exit code) instead of swallowing it so permission/auth/session problems are
visible early.
- Around line 369-376: The invoke datamanager flow calls
tools/sync_equity_categories.py but never sets the required environment
variables AWS_S3_DATA_BUCKET_NAME and MASSIVE_API_KEY, so the script will
sys.exit(1) and kill the parent; before running sync_equity_categories.py in the
invoke path, fetch/assign the same environment values used in the “data sync
equity categories” flow (populate AWS_S3_DATA_BUCKET_NAME and MASSIVE_API_KEY
from the secrets/lookup used elsewhere) and export them into the environment so
sync_equity_categories.py sees them and does not exit.
chrisaddy
previously approved these changes
Feb 19, 2026
forstmeier
dismissed stale reviews from coderabbitai[bot] and chrisaddy
via
7146f30
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@maskfile.md`:
- Line 750: The pulumi output calls in the export lines (e.g., the line
exporting AWS_S3_DATA_BUCKET_NAME that runs "pulumi stack output
aws_s3_data_bucket_name") must explicitly target the production stack; update
each pulumi stack output invocation used by the data sync, models prepare, and
models train commands to include "--stack production" (also apply same change to
the nearby occurrences referenced at the other export lines). Locate the export
lines (search for "export AWS_S3_DATA_BUCKET_NAME" and other similar exports
calling "pulumi stack output") and add the "--stack production" flag to each
pulumi invocation so they always read outputs from the production stack.
---
Duplicate comments:
In `@maskfile.md`:
- Around line 198-215: The import block is running on every `pulumi up` (because
imports were moved out of the BOOTSTRAP guard), causing extra AWS API calls and
hiding real errors; restore a lightweight guard: before calling each `pulumi
import` (the commands referencing github_actions_infrastructure_role,
github_actions_infrastructure_policy, sagemaker_execution_role,
sagemaker_execution_policy and the aws iam list-policies lookups for
GITHUB_POLICY_ARN and SAGEMAKER_POLICY_ARN) first check whether the resource is
already present in the Pulumi state (e.g., via a pulumi state/stack query or
other existence check) and only run `pulumi import` if absent, and stop
swallowing errors with `2>/dev/null || true` on the import commands so
permission/auth failures surface instead of being silently ignored;
alternatively rewrap the block under the original BOOTSTRAP flag if the intent
was to run imports only during bootstrapping.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@maskfile.md`:
- Around line 198-214: The import commands currently suppress all errors using
"2>/dev/null || true", hiding auth/permission or other failures; update the
pulumi import invocations (the lines calling pulumi import for
github_actions_infrastructure_role, github_actions_infrastructure_policy using
GITHUB_POLICY_ARN, sagemaker_execution_role, and sagemaker_execution_policy
using SAGEMAKER_POLICY_ARN) to stop discarding stderr and instead
capture/inspect the command result: remove the blanket "2>/dev/null || true",
redirect stderr to a variable or a temp log, check the exit code and only ignore
the import when the error text indicates the resource is already
imported/idempotent, otherwise emit the captured stderr and fail (or surface a
clear warning) so auth/permission failures are visible.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: maskfile.md
Line: 318
Comment:
The `down` command uses `--stack production` while the `up` command now uses `--stack ${organization_name}/fund/production`. Should apply same organization logic here for consistency.
```suggestion
if ! organization_name=$(pulumi org get-default 2>/dev/null) || [ -z "${organization_name}" ]; then
echo "Unable to determine Pulumi organization name - ensure you are logged in"
exit 1
fi
pulumi stack select ${organization_name}/fund/production
pulumi down --yes
```
How can I resolve this? If you propose a fix, please make it concise. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@maskfile.md`:
- Around line 352-355: The curl call that sets http_code by POSTing to
"$full_url" can hang indefinitely; update the curl invocation in that block (the
command assigning http_code) to include a bounded timeout like --max-time
<seconds> (and optionally --connect-timeout <seconds>) and add --silent to
suppress progress output so stderr only contains the response body; ensure these
flags are applied to the same curl invocation that writes out "%{http_code}" and
outputs to /dev/stderr.
---
Duplicate comments:
In `@maskfile.md`:
- Around line 793-801: This block duplicates the Pulumi stack-select guard
already implemented earlier; refactor by extracting the organization lookup and
stack selection into a shared helper (e.g., a function that runs pulumi org
get-default, validates organization_name, and invokes pulumi stack select) and
replace this duplicated snippet with a call to that helper; reference the
organization_name variable, the pulumi org get-default invocation, and the
pulumi stack select ${organization_name}/fund/production so the fix updates both
occurrences to use the same centralized logic.
- Around line 869-878: This block duplicates the earlier Pulumi stack-select
guard — remove the redundant guard and stack selection to avoid repeated checks
and potential exits; keep a single, well-placed guard that determines
organization_name (the organization_name variable from the pulumi org
get-default call) and a single pulumi stack select invocation, and ensure
downstream export lines (AWS_ECR_EQUITY_PRICE_MODEL_TRAINER_IMAGE_ARN,
AWS_IAM_SAGEMAKER_ROLE_ARN, AWS_S3_MODEL_ARTIFACTS_BUCKET_NAME) rely on that
single guard/selection instead of re-duplicating the same logic.
- Around line 748-756: The review notes that the missing stack selection risk is
resolved because pulumi stack select ${organization_name}/fund/production is now
run before every pulumi stack output (e.g., around the export of
AWS_S3_DATA_BUCKET_NAME), and the comment is duplicated; remove the
duplicate_review marker and/or the duplicated comment block so only one review
comment remains (or mark the earlier comment as resolved), ensuring the code
sections that call pulumi stack select and pulumi stack output (the blocks
referencing organization_name, pulumi stack select, and export
AWS_S3_DATA_BUCKET_NAME) remain unchanged.
- Around line 196-212: The pulumi import calls are suppressing all errors with
"2>/dev/null || true"; instead, remove that suppression and only run imports
when the resource is known to exist (use the same pattern already used for
policies): check existence before importing (e.g., query AWS for the role ARN or
use aws iam get-role/name) and then call the matching pulumi import for
github_actions_infrastructure_role and sagemaker_execution_role; also drop the
blanket "2>/dev/null || true" on the policy imports and rely on the
GITHUB_POLICY_ARN and SAGEMAKER_POLICY_ARN checks you already have to
conditionally run pulumi import so failures surface.
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
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.
Overview
Changes
Context
I've run the
--bootstrapflag locally so stuff in GitHub and AWS should be present and correct for us to do the GitHub actions stuff whenever we eventually re-enable it. Until then, we can continue using "up" without the flag locally.Summary by CodeRabbit
New Features
Chores
Bug Fixes