Add namespace support for S3 bucket naming and configuration#166
Conversation
…tion Enhanced the deployment workflows (`deploy-s3.yml`, `prod.yml`, `staging.yml`) to include a namespace parameter for S3 bucket names, ensuring uniqueness for forks. Updated the CloudFormation template (`s3.yaml`) to conditionally create bucket names based on the namespace. Modified the AWS setup scripts (`aws.sh`, `bootstrap.sh`) to handle the new namespace configuration and adjusted the environment configuration (`env.py`, `secrets_manager.py`) to reflect these changes. Updated documentation to clarify the new namespace behavior for forks.
|
@claude please review this PR |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Claude finished @jfrench9's task in 1m 36s —— View job PR Review: Namespace Support for S3 Bucket NamingConducting a comprehensive review of the namespace implementation across all changed files. Progress
Review SummaryThis PR successfully implements namespace support for S3 bucket naming to enable better resource isolation between the main RoboSystems deployment and forks. The implementation is generally solid with good consistency across infrastructure and application layers. Overall Assessment: Approve with minor suggestions Strengths
Issues & Recommendations1. CloudFormation Pattern Inconsistency (cloudformation/s3.yaml)Issue: The namespace parameter allows hyphens but the computed bucket names insert the namespace with a hyphen, which could create double-hyphens if someone passes a namespace with trailing/leading hyphens. Location: cloudformation/s3.yaml:15-19 AllowedPattern: "^[a-z0-9-]*$"Location: cloudformation/s3.yaml:70-71 - !Sub "robosystems-${Namespace}-shared-raw-${Environment}"Recommendation:
Example fix: AllowedPattern: "^[0-9]{12}$|^$"
ConstraintDescription: Must be empty or a 12-digit AWS account ID2. Missing Error Handling in Bootstrap (bin/setup/bootstrap.sh)Issue: The S3_NAMESPACE is exported and used in aws.sh script, but there's no validation that the script actually received it. Location: bin/setup/bootstrap.sh:504-508 else
export S3_NAMESPACE="$AWS_ACCOUNT_ID"
print_success "Fork detected - using account ID as namespace"Recommendation: Add validation in bin/setup/aws.sh to ensure S3_NAMESPACE is set when expected: # After line 82 in aws.sh
if [ "$full_repo" != "RoboFinSystems/robosystems" ] && [ -z "${S3_NAMESPACE:-}" ]; then
echo "ERROR: S3_NAMESPACE must be set for forked deployments"
exit 1
fi3. Potential Configuration Drift (robosystems/config/env.py)Issue: The bucket name computation logic exists in three places with slightly different implementations:
Location: Multiple files
Recommendation: # In robosystems/config/storage/buckets.py (new file)
def compute_bucket_name(purpose: str, namespace: str = "", environment: str = "dev") -> str:
"""
Compute S3 bucket name with consistent logic.
Args:
purpose: Bucket purpose (shared-raw, shared-processed, user, etc.)
namespace: Optional namespace (AWS account ID for forks)
environment: Environment name (dev, staging, prod)
Returns:
Computed bucket name following the pattern:
robosystems-{namespace}-{purpose}-{env} or robosystems-{purpose}-{env}
"""
suffix = "" if environment == "dev" else f"-{environment}"
prefix = f"robosystems-{namespace}" if namespace else "robosystems"
return f"{prefix}-{purpose}{suffix}"This would eliminate duplication and reduce drift risk. 4. Documentation Gap in WorkflowsIssue: The namespace auto-detection logic is clever but not well-documented in the workflow files themselves. Location:
Current: # Namespace Configuration (auto: main repo = none, forks = account ID)
namespace: ${{ github.repository == 'RoboFinSystems/robosystems' && '' || vars.AWS_ACCOUNT_ID }}Recommendation: Expand the comment to explain the S3 uniqueness requirement: # Namespace Configuration (auto-computed for S3 global uniqueness)
# Main repo (RoboFinSystems/robosystems): empty namespace → robosystems-shared-raw-prod
# Forks: AWS account ID → robosystems-{account-id}-shared-raw-prod
# This ensures globally unique S3 bucket names across all AWS accounts
namespace: ${{ github.repository == 'RoboFinSystems/robosystems' && '' || vars.AWS_ACCOUNT_ID }}5. Minor: Unused Variable in aws.shIssue: The s3_namespace_entry variable construction could be simplified. Location: bin/setup/aws.sh:82-86, 162-167 Current: local s3_namespace_entry=""
if [ -n "${S3_NAMESPACE:-}" ]; then
s3_namespace_entry="\"S3_NAMESPACE\": \"${S3_NAMESPACE}\","
echo "Including S3_NAMESPACE: ${S3_NAMESPACE}"
fiRecommendation: This is actually well-implemented. No change needed - the echo is useful for user feedback. Testing RecommendationsGiven the breaking changes to S3 bucket naming:
Migration ConsiderationsBreaking Change Impact: This PR changes the default bucket naming convention. For existing deployments:
Migration Path for Existing Forks:
Consider adding a migration script or at least a clear migration guide in the PR description. Security Review✅ No security issues identified:
Code QualityStrengths:
Minor Improvements:
ConclusionThis is a well-executed feature that addresses a real need for fork deployments. The implementation is consistent and follows AWS best practices for resource naming. The suggested improvements are minor and don't block merging. Recommendation: Approve and merge with the understanding that the CloudFormation parameter validation should be tightened in a follow-up PR if needed. Great work on maintaining consistency across all the layers! 🎉 |
Updated the `test_secrets_manager.py` to include additional assertions for S3 bucket names in both dev and staging environments. Added tests for handling namespace in bucket names for fork deployments, ensuring correct naming conventions based on the environment and namespace settings. This improves coverage and validation of the S3 bucket naming logic.
Simplified the `LOGS_BUCKET` assignment in `env.py` for better readability by condensing the multi-line statement into a single line. This change enhances the clarity of the environment configuration. Additionally, updated assertions in `test_secrets_manager.py` to format the `shared_processed` bucket name check for consistency and readability.
Added validation for the S3_NAMESPACE environment variable in the AWS setup script to ensure it is a 12-digit AWS account ID. Updated the CloudFormation template to enforce this format. Refactored bucket name generation logic into a new helper module to centralize and simplify the computation of S3 bucket names across the application, ensuring consistency in naming conventions for different environments and deployments.
Modified the AWS setup script to replace hardcoded redirect URIs and user agent strings with placeholders for better configurability. Refactored S3 bucket naming logic to remove deprecated aliases and improve clarity in purpose mapping. Updated tests to reflect these changes, ensuring consistency in bucket name assertions across environments.
1 participant
Summary
This PR introduces namespace support across the AWS infrastructure to enable better resource isolation and multi-environment deployments. The changes primarily focus on S3 bucket naming conventions and related configuration management.
Key Accomplishments
Infrastructure Considerations
Breaking Changes
Testing Notes
This change improves the system's ability to support multiple isolated environments while maintaining clear resource boundaries.
🤖 Generated with Claude Code
Branch Info:
chore/even-more-bootstrap-refinementsmainCo-Authored-By: Claude noreply@anthropic.com