-
Notifications
You must be signed in to change notification settings - Fork 47
feat: Add setup script for Vertex AI on kind clusters #555
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Claude Code Review - PR 555Overall Assessment: Ready to merge with minor recommendations SummaryThis PR adds a helper script to configure Vertex AI for kind clusters. The script is well-written, secure, and follows established patterns. Issues FoundMajor Issues (1):
Minor Issues (4):
Positive HighlightsExcellent Documentation: Script header is exemplary with clear prerequisites, env vars, examples, and verification steps Security Best Practices:
Consistent Design:
Code Quality: 8.7/10
RecommendationsBefore merge: Add Makefile target Follow-up PRs:
ConclusionRecommendation: APPROVE No blocking issues. Script demonstrates strong engineering practices. Minor improvements can be addressed in follow-ups. Reviewed by Claude Sonnet 4.5 | Standards: CLAUDE.md, Security Standards, Error Handling Patterns |
scripts/setup-vertex-kind.sh
Outdated
| echo "" | ||
| echo "Next steps:" | ||
| echo " 1. Verify the operator started correctly:" | ||
| echo " kubectl logs -l app=agentic-operator -n $NAMESPACE | grep -i vertex" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it doesn't generate such a log immediately; you can't use this (at this time) to verify that the operator started correctly. I think you'd see this only after creating a session.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm seeing the same - no output when I run this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if you need to wait a bit or if you need to create a new session. I added sleep over there in case it takes some time for the operator
scripts/setup-vertex-kind.sh
Outdated
| \"CLAUDE_CODE_USE_VERTEX\": \"1\", | ||
| \"ANTHROPIC_VERTEX_PROJECT_ID\": \"$ANTHROPIC_VERTEX_PROJECT_ID\", | ||
| \"CLOUD_ML_REGION\": \"$CLOUD_ML_REGION\", | ||
| \"GOOGLE_APPLICATION_CREDENTIALS\": \"/etc/vertex/ambient-code-key.json\" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should that be /app/vertex/... instead of /etc/vertex/...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, fixed
Adds a helper script to configure the Ambient Code Platform to use Google Cloud Vertex AI instead of the Anthropic API when running locally on kind. Uses the same environment variables as Claude Code CLI (GOOGLE_APPLICATION_CREDENTIALS, ANTHROPIC_VERTEX_PROJECT_ID, CLOUD_ML_REGION) for consistency. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Address PR review feedback: - Fix GOOGLE_APPLICATION_CREDENTIALS path from /etc/vertex/ to /app/vertex/ to match actual mount path in operator (sessions.go:1222) - Add automatic verification step that checks operator logs for Vertex mode activation instead of just printing a manual command Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
30c800d to
319dd8d
Compare
Claude Code ReviewSummaryThis PR adds a helpful script to configure Vertex AI on kind clusters. The script is well-documented, follows bash best practices, and provides a good user experience with clear validation and feedback. The implementation correctly aligns with the existing Vertex AI configuration in the operator and Makefile. Overall Assessment: ✅ Ready to merge with minor suggestions for improvement. Issues by Severity🚫 Blocker IssuesNone. 🔴 Critical IssuesNone. 🟡 Major Issues1. Missing Volume Mount Configuration in Operator Deployment The script configures the ConfigMap with Impact: The operator's preflight validation at Evidence:
Recommended Fix: This is actually a design issue in the operator's preflight validation, not this script. The preflight check should either:
For this PR: Add a note in the script's comments explaining that the operator preflight warnings about the credentials file are expected and can be ignored (the file is mounted to runners, not the operator). 🔵 Minor Issues1. Script Name Doesn't Match Pattern (Line 1) Location: Issue: Other setup scripts use different naming patterns:
This script is named Recommendation: Rename to 2. Documentation Gap Issue: The script isn't mentioned in the kind development documentation at Recommendation: Add a section to the kind.md docs: ## Vertex AI Configuration (Optional)
If you have Google Cloud Vertex AI access instead of a direct Anthropic API key:
```bash
# Export environment variables (add to ~/.zshrc)
export GOOGLE_APPLICATION_CREDENTIALS="$HOME/.config/gcloud/your-sa-key.json"
export ANTHROPIC_VERTEX_PROJECT_ID="your-gcp-project-id"
export CLOUD_ML_REGION="us-east5"
# Run setup script
./scripts/setup-vertex-kind.shSee the script header for detailed setup instructions. 4. No Validation of GCP Credentials File Format Location: Lines 92-99 Issue: Script checks if file exists but doesn't validate it's valid JSON or has required fields. Risk: Low - invalid credentials will fail at runtime with clear errors from the GCP SDK. Recommendation (optional): Add basic JSON validation: elif \! jq -e '.type' "$GOOGLE_APPLICATION_CREDENTIALS" >/dev/null 2>&1; then
echo " [ERROR] GOOGLE_APPLICATION_CREDENTIALS file is not valid JSON"
missing_vars=1Requires 5. Verification Step May Fail Silently Location: Lines 172-178 Issue: The verification check looks for "vertex ai mode enabled" in logs, but:
Recommendation: Make the verification optional/informational: echo "Verifying Vertex AI configuration (may take a few seconds)..."
sleep 5
if kubectl logs -l app=agentic-operator -n "$NAMESPACE" --tail=200 2>/dev/null | grep -qi "vertex"; then
echo " ✓ Vertex-related logs found in operator"
else
echo " ℹ Verification skipped (operator may still be starting)"
echo " Manually verify: kubectl logs -l app=agentic-operator -n $NAMESPACE | grep -i vertex"
fiPositive Highlights1. Excellent Documentation (Lines 1-52) The script header is exemplary:
This is exactly the level of documentation scripts should have. 2. Proper Error Handling
3. Consistent with Existing Patterns The script correctly mirrors the Makefile's
4. Good User Experience
5. Help Flag Support Lines 56-60 provide a nice RecommendationsPriority 1: Documentation
Priority 2: Robustness
Priority 3: Polish (Optional)
Testing SuggestionsBefore merging, verify:
Code Quality Score: 8.5/10 Strong implementation with excellent documentation. The major issue is actually in the operator's preflight validation design, not this script. With documentation additions, this is ready to merge. 🔍 View AI decision process (logs available for 90 days) 📋 View memory system files loaded (click to expand)What Amber Loaded for Code ReviewAmber automatically loaded these repository standards from the memory system:
Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines. |
Claude Code ReviewThis PR adds a helper script to configure Vertex AI for kind clusters. The script is well-documented and follows established patterns. Critical Issue FoundOperator Deployment Volume Mount Missing The script sets GOOGLE_APPLICATION_CREDENTIALS=/app/vertex/ambient-code-key.json but components/manifests/base/operator-deployment.yaml does NOT mount the ambient-vertex secret at /app/vertex. Impact: Vertex AI mode will fail at operator startup when preflight.ValidateVertexConfig checks if the credentials file exists. Fix: Add volumeMounts and volumes to operator-deployment.yaml to mount the ambient-vertex secret at /app/vertex. Positive Highlights
RecommendationApprove with changes requested. The script is well-written but the missing operator volume mount must be addressed before merge. Estimated effort: 10 minutes. Full detailed review with 7 issues and recommendations available upon request. 🔍 View AI decision process (logs available for 90 days) 📋 View memory system files loaded (click to expand)What Amber Loaded for Code ReviewAmber automatically loaded these repository standards from the memory system:
Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines. |
Adds a helper script to configure the Ambient Code Platform to use Google Cloud Vertex AI instead of the Anthropic API when running locally on kind.
Uses the same environment variables as Claude Code CLI (GOOGLE_APPLICATION_CREDENTIALS, ANTHROPIC_VERTEX_PROJECT_ID, CLOUD_ML_REGION) for consistency.