Add new bucket #5309
Add new bucket #5309
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
|
Claude finished @H2Shami's task —— View job Code Review CompleteScore: 8/10 - Good to merge, minor suggestions for improvement Suggestions Summary:
Detailed ReviewTodo List:
Security & Performance Assessment✅ Strong Points:
✅ Code Quality:
Minor Improvement Suggestions1. Environment Variable Fallback (PromptManager.ts:48) // Current
S3_PROMPT_BUCKET_NAME: process.env.S3_PROMPT_BUCKET_NAME ?? "",
// Suggested improvement
S3_PROMPT_BUCKET_NAME: process.env.S3_PROMPT_BUCKET_NAME ?? process.env.S3_BUCKET_NAME ?? "",This provides a fallback to the main bucket if the prompt bucket isn't configured, preventing potential runtime errors. 2. Enhanced Minio Setup (supervisord.conf:83) # Current setup command only creates request-response-storage bucket
# Suggested: Include prompt bucket creation
command=/bin/sh -c "until curl -f http://localhost:9080/minio/health/live > /dev/null 2>&1; do echo 'Waiting for MinIO...'; sleep 2; done && /usr/local/bin/mc alias set localminio http://localhost:9080 minioadmin minioadmin && /usr/local/bin/mc mb --ignore-existing localminio/request-response-storage && /usr/local/bin/mc mb --ignore-existing localminio/prompt-body-storage && exit 0"3. Configuration Validation Breaking Changes Assessment✅ Well Managed:
Infrastructure & Deployment✅ Comprehensive Configuration:
✅ Regional Configuration: Final AssessmentThis PR implements a clean separation of prompt storage from request/response storage. The implementation is thorough, consistent across environments, and follows good practices. The breaking change is well-documented and managed appropriately. The suggested improvements are minor optimizations and don't affect the core functionality. The code is ready for production deployment with proper environment variable configuration. --- | Branch: |
Greptile OverviewGreptile SummaryThis PR adds a new dedicated S3 bucket (
Confidence Score: 5/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant Client
participant Worker
participant Env Config
participant PromptManager
participant S3 Prompt Bucket
participant S3 Request Bucket
Note over Client,S3 Request Bucket: New Bucket Routing Flow
Client->>Worker: Request (EU or US region)
Worker->>Env Config: Check if EU request
alt EU Request
Env Config->>Env Config: Set S3_PROMPT_BUCKET_NAME = EU_S3_PROMPT_BUCKET_NAME
Env Config->>Env Config: Set S3_BUCKET_NAME = EU_S3_BUCKET_NAME
else US Request
Env Config->>Env Config: Use default S3_PROMPT_BUCKET_NAME
Env Config->>Env Config: Use default S3_BUCKET_NAME
end
Worker->>PromptManager: Initialize with environment
PromptManager->>PromptManager: Create S3Client(S3_PROMPT_BUCKET_NAME)
Note over PromptManager,S3 Request Bucket: Separate Storage
PromptManager->>S3 Prompt Bucket: Store/Retrieve Prompt Data
Note right of S3 Prompt Bucket: prompt-body-storage
Worker->>S3 Request Bucket: Store/Retrieve Request/Response Data
Note right of S3 Request Bucket: request-response-storage
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| STRIPE_WEBHOOK_SECRET: string; | ||
| HELICONE_ORG_ID: string; | ||
| EU_HELICONE_ORG_ID: string; | ||
| STRIPE_CLOUD_GATEWAY_TOKEN_USAGE_PRODUCT: string; | ||
| UPSTASH_KAFKA_URL: string; | ||
| UPSTASH_KAFKA_USERNAME: string; | ||
| UPSTASH_KAFKA_PASSWORD: string; | ||
| QUEUE_PROVIDER: string; | ||
| STRIPE_WEBHOOK_SECRET: string; | ||
| HELICONE_ORG_ID: string; |
There was a problem hiding this comment.
why's this change happening? I think its what is causing the worker test to fail
There was a problem hiding this comment.
you have it in your dev.vars. just comment them out before generating the types plz
| EU_S3_BUCKET_NAME = "request-response-storage" | ||
| EU_S3_PROMPT_BUCKET_NAME = "prompt-body-storage" |
There was a problem hiding this comment.
for this, i just followed the EU_S3_BUCKET_NAME, but shouldn't these have -eu appended to the end?
There was a problem hiding this comment.
i think worker doesn't use the prompt bucket? so it doesn't matter
There was a problem hiding this comment.
if the worker doesnt let's remove these chnages then
| networks: | ||
| traces: | ||
| driver: bridge | ||
| driver: bridge |
connortbot
left a comment
connortbot
left a comment
There was a problem hiding this comment.
LGTM but need to move prompt files from bucket first
Ticket
Link to the ticket(s) this pull request addresses.
Component/Service
What part of Helicone does this affect?
Type of Change
Testing
Technical Considerations
Dependencies
Deployment Notes
Context
prevents prompts from getting deleted after 90 days along with requests
Misc. Review Notes
Add
S3_PROMPT_BUCKET_NAME="prompt-body-storage"to your jawn env file