GCP MCP integration - #1310
GCP MCP integration#1310
Conversation
|
WalkthroughAdds Google Cloud Platform (GCP) MCP support: Helm templates (helpers, deployment, networkpolicy), values and toolset-config integration, new GCP docs/nav/index entries, and a small MCP plugin change for CLI/gcloud argument formatting. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer (values.yaml)
participant Helm as Helm templates
participant K8s as Kubernetes API
participant MCP as GCP MCP Pods
participant Holmes as Holmes app
Dev->>Helm: provide .Values.mcpAddons.gcp + llmInstructions
Helm->>K8s: render & apply ConfigMap, ServiceAccount, Deployment, Service, NetworkPolicy
K8s->>MCP: schedule Pods (gcloud / observability / storage)
Helm->>Holmes: render toolset-config merging gcp_* into mcp_servers
Holmes->>MCP: call MCP endpoints (commands formatted by toolset_mcp.get_parameterized_one_liner)
MCP-->>Holmes: responses / outputs
Note right of Helm: llmInstructions included in generated toolset config
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
|
✅ Docker image ready for
Use this tag to pull the image for testing. 📋 Copy commandsgcloud auth configure-docker us-central1-docker.pkg.dev
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:51ea980
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:51ea980 me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:51ea980
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:51ea980Patch Helm values in one line (choose the chart you use): HolmesGPT chart: helm upgrade --install holmesgpt ./helm/holmes \
--set registry=me-west1-docker.pkg.dev/robusta-development/development \
--set image=holmes-dev:51ea980Robusta wrapper chart: helm upgrade --install robusta robusta/robusta \
--reuse-values \
--set holmes.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.image=holmes-dev:51ea980 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
holmes/plugins/toolsets/mcp/toolset_mcp.py (1)
188-190: Minor redundancy in f-string.The
params.get('cli_command')is already checked in the condition, so you can use it directly.🔎 Suggested simplification
# AWS MCP cli_command if params and params.get("cli_command"): - return f"{params.get('cli_command')}" + return params["cli_command"]helm/holmes/values.yaml (1)
222-227: Resource limits are inconsistent across GCP services.The
gcloudservice has a CPU limit defined in requests but is missing it in limits, whileobservabilityandstoragehave only memory limits. Consider adding CPU limits for consistency and better resource management:🔎 Suggested resource consistency
gcloud: enabled: true image: "gcloud-cli-mcp:1.0.7" port: 8000 resources: requests: memory: "256Mi" cpu: "100m" limits: memory: "1Gi" + cpu: "500m"Apply similar CPU limits to
observabilityandstorageservices for consistency.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 0eced48 and 1d25e71229460a48f22e4434ec8ad30f1809d667.
📒 Files selected for processing (8)
docs/data-sources/builtin-toolsets/.nav.ymldocs/data-sources/builtin-toolsets/index.mdhelm/holmes/templates/mcp-servers/gcp/_helpers.tplhelm/holmes/templates/mcp-servers/gcp/deployment.yamlhelm/holmes/templates/mcp-servers/gcp/networkpolicy.yamlhelm/holmes/templates/toolset-config.yamlhelm/holmes/values.yamlholmes/plugins/toolsets/mcp/toolset_mcp.py
🧰 Additional context used
📓 Path-based instructions (3)
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
When writing documentation in the docs/ directory, always add a blank line between a header/bold text and a list, otherwise MkDocs won't render the list properly
Files:
docs/data-sources/builtin-toolsets/index.md
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml)
Type hints required (mypy configuration in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
holmes/plugins/toolsets/mcp/toolset_mcp.py
holmes/plugins/toolsets/**
📄 CodeRabbit inference engine (CLAUDE.md)
Toolsets: organize as
holmes/plugins/toolsets/{name}.yamlor{name}/directories
Files:
holmes/plugins/toolsets/mcp/toolset_mcp.py
🧠 Learnings (1)
📚 Learning: 2025-12-29T08:35:37.678Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T08:35:37.678Z
Learning: New toolsets require integration tests
Applied to files:
docs/data-sources/builtin-toolsets/index.md
🪛 YAMLlint (1.37.1)
helm/holmes/templates/mcp-servers/gcp/networkpolicy.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
helm/holmes/templates/toolset-config.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
helm/holmes/templates/mcp-servers/gcp/deployment.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🔇 Additional comments (12)
holmes/plugins/toolsets/mcp/toolset_mcp.py (1)
192-198: Logic looks good for gcloud command generation.The implementation correctly handles the
run_gcloud_commandtool by joining args into a propergcloudCLI command. Theisinstancecheck provides good defensive typing for the args parameter.docs/data-sources/builtin-toolsets/index.md (1)
29-29: MariaDB (MCP) entry looks good.Consistent with other entries in the index.
helm/holmes/templates/mcp-servers/gcp/networkpolicy.yaml (2)
52-56: Egress rule is permissive by design.The egress allows all external traffic except the GCP metadata service. While this is broad, it's pragmatic for GCP API access since Google's IP ranges change frequently. The metadata service block (
169.254.169.254/32) is a good security measure to enforce Workload Identity.
22-41: Ingress rules correctly scoped to Holmes pods.The conditional port exposure based on enabled flags is well-implemented.
helm/holmes/values.yaml (1)
184-262: GCP MCP configuration structure is comprehensive.The configuration provides good flexibility with:
- Multiple authentication methods (service account key, workload identity)
- Per-service toggles and resource controls
- Placement options (nodeSelector, tolerations, affinity)
- LLM instruction overrides
The documentation comments explaining authentication setup and multi-project support are helpful.
helm/holmes/templates/mcp-servers/gcp/deployment.yaml (3)
91-96: Good security context configuration.Running as non-root user (1000) with proper fsGroup is a security best practice.
206-273: Observability container configuration looks correct.The container properly sets
GOOGLE_APPLICATION_CREDENTIALSwhen service account key is provided, includes ConfigMap references for project/region, and has appropriate health probes.
343-379: Service definition correctly exposes all enabled MCP ports.The conditional port exposure matches the container definitions.
helm/holmes/templates/toolset-config.yaml (1)
33-69: GCP MCP server configuration block is well-structured.The implementation follows the established pattern from AWS and MariaDB MCP servers. The conditional merging of each subcomponent (gcloud, observability, storage) is clean and allows granular control. All referenced helper templates (holmes.gcpMcp.gcloud.llmInstructions, holmes.gcpMcp.observability.llmInstructions, holmes.gcpMcp.storage.llmInstructions) are properly defined in helm/holmes/templates/mcp-servers/gcp/_helpers.tpl.
helm/holmes/templates/mcp-servers/gcp/_helpers.tpl (3)
4-56: Well-structured template with comprehensive gcloud guidance.The template definition follows proper Helm patterns and provides clear, detailed instructions for LLM consumption. The emphasis on args array format (excluding 'gcloud' prefix) and multi-project support with explicit
--projectflags is particularly valuable.
61-111: Excellent observability instructions with strong link guidance.The template provides comprehensive guidance for GCP observability with particularly strong emphasis on:
- Historical data retrieval for deleted resources (lines 67-68)
- Descriptive link naming conventions (lines 79-85)
- Practical filter patterns for common scenarios
The critical instruction at line 79 to "always answer log questions with actual log entries first" is especially valuable for ensuring LLMs provide actionable results before offering console links.
116-147: Clear and practical storage instructions.The template provides focused guidance for Cloud Storage investigations with logical troubleshooting flows. The investigation patterns (lines 136-141) are particularly helpful, offering a clear sequence for common issues like access denied and missing data.
|
fixes #1195 |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
helm/holmes/values.yaml (2)
162-163: Placeholder URLs should be updated.Lines 163 and 196 contain placeholder URLs (
https://github.com/your-org/holmes-mcp-integrations) that should be replaced with the actual repository location or removed if images are already published.Also applies to: 196-196
261-309: Azure MCP templates are still missing.The Azure MCP values are defined here, but corresponding Helm templates are not present in
helm/holmes/templates/mcp-servers/azure/. These values will have no effect without the associated templates.
🧹 Nitpick comments (2)
helm/holmes/values.yaml (2)
219-248: Consider adding CPU limits for consistency with other MCP addons.The GCP MCP servers define CPU requests but omit CPU limits, while AWS and MariaDB addons in the same file specify both. Without CPU limits, containers can consume unbounded CPU, potentially causing resource contention.
🔎 Suggested addition of CPU limits
gcloud: enabled: true image: "gcloud-cli-mcp:1.0.7" port: 8000 resources: requests: memory: "256Mi" cpu: "100m" limits: memory: "1Gi" + cpu: "500m" # Observability MCP - Cloud Logging, Monitoring, Trace, Error Reporting observability: enabled: true image: "gcloud-observability-mcp:1.0.0" port: 8001 resources: requests: memory: "256Mi" cpu: "100m" limits: memory: "512Mi" + cpu: "250m" # Storage MCP - Cloud Storage operations storage: enabled: true image: "gcloud-storage-mcp:1.0.0" port: 8002 resources: requests: memory: "256Mi" cpu: "100m" limits: memory: "512Mi" + cpu: "250m"
288-293: Azure resources also lack CPU limits.Same observation as the GCP section: CPU requests are specified but limits are omitted. For consistency with AWS and MariaDB addons, consider adding CPU limits.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 1d25e71229460a48f22e4434ec8ad30f1809d667 and 76140490295301514bfecd489e5a4d034d97d4ca.
📒 Files selected for processing (3)
docs/data-sources/builtin-toolsets/index.mdhelm/holmes/values.yamlholmes/plugins/toolsets/mcp/toolset_mcp.py
🚧 Files skipped from review as they are similar to previous changes (2)
- holmes/plugins/toolsets/mcp/toolset_mcp.py
- docs/data-sources/builtin-toolsets/index.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: llm_evals
✅ Results of HolmesGPT evalsAutomatically triggered by commit 7614049 on branch Results of HolmesGPT evals
Time/Cost columns show % change vs historical average (↑slower/costlier, ↓faster/cheaper). Changes under 10% shown as ±0%. Historical Comparison DetailsFilter: excluding branch 'gcloud-mcp' Status: Success - 22 test/model combinations loaded Experiments compared (30):
Comparison indicators:
📖 Legend
🔄 Re-run evals manually
Option 1: Comment on this PR with Or with more options (one per line): Run evals on a different branch (e.g., master) for comparison:
Quick re-run: Use Option 2: Trigger via GitHub Actions UI → "Run workflow" 🏷️ Valid markers
Commands: |
✅ Results of HolmesGPT evalsAutomatically triggered by commit 2dd1dfd on branch 📖 Legend
🔄 Re-run evals manually
Option 1: Comment on this PR with Or with more options (one per line): Run evals on a different branch (e.g., master) for comparison:
Quick re-run: Use Option 2: Trigger via GitHub Actions UI → "Run workflow" 🏷️ Valid markers
Commands: |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
helm/holmes/values.yaml (1)
163-163: Placeholder URLs remain unfixed.Both references to
https://github.com/your-org/holmes-mcp-integrationsare placeholders that should be updated to the actual repository location before merging.Also applies to: 196-196
🧹 Nitpick comments (3)
helm/holmes/values.yaml (3)
219-225: Add CPU limit for consistency and resource isolation.The
gcloudservice defines a CPU limit, but it's missing from the YAML. Theobservability(line 236) andstorage(line 248) services both specifycpulimits underlimits. Adding a CPU limit prevents unbounded CPU consumption and aligns with Kubernetes best practices.🔎 Suggested fix
resources: requests: memory: "256Mi" cpu: "100m" limits: memory: "1Gi" + cpu: "500m"
231-237: Add CPU limit for resource isolation.The
observabilityservice is missing acpulimit. Adding one prevents unbounded CPU consumption and follows Kubernetes best practices for resource management.🔎 Suggested fix
resources: requests: memory: "256Mi" cpu: "100m" limits: memory: "512Mi" + cpu: "500m"
243-249: Add CPU limit for resource isolation.The
storageservice is missing acpulimit. Adding one prevents unbounded CPU consumption and follows Kubernetes best practices for resource management.🔎 Suggested fix
resources: requests: memory: "256Mi" cpu: "100m" limits: memory: "512Mi" + cpu: "500m"
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 76140490295301514bfecd489e5a4d034d97d4ca and 2dd1dfd80325d63459996e63089eb7f845c9a86d.
📒 Files selected for processing (1)
helm/holmes/values.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: llm_evals
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
- GitHub Check: build (3.10)
🔇 Additional comments (2)
helm/holmes/values.yaml (2)
254-259: The nestedllmInstructionsstructure is correctly implemented. The three GCP services (gcloud, observability, storage) each have dedicated template definitions in_helpers.tplthat properly check for nested overrides at.Values.mcpAddons.gcp.llmInstructions.{gcloud|observability|storage}, with sensible defaults provided when overrides are empty. All three templates are properly included intoolset-config.yamlwith appropriate filters. This nested design is appropriate for GCP's multi-service architecture (unlike AWS and MariaDB's single-service flat structure).
189-189: Remove or update the Workload Identity gcloud CLI limitation comment—it's inaccurate.Workload Identity DOES work with gcloud CLI in GKE. Pods with Workload Identity enabled can authenticate using
gcloud auth print-access-tokenor Application Default Credentials. Workload Identity Federation also supports gcloud CLI (v363.0.0+, available as of Jan 2026). The claim that it "currently not working with gcloud CLI" is incorrect and should be removed or clarified.Also applies to: 200-202
Likely an incorrect or invalid review comment.
Signed-off-by: Arik Alon <alon.arik@gmail.com>
Signed-off-by: Arik Alon <alon.arik@gmail.com>
Signed-off-by: Arik Alon <alon.arik@gmail.com>
Signed-off-by: Arik Alon <alon.arik@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/data-sources/builtin-toolsets/gcp.md (1)
339-355: Consider indented code blocks in numbered lists for better Markdown style consistency.Fenced code blocks (triple backticks) inside numbered lists can sometimes cause rendering issues. Markdownlint prefers indented code blocks (4+ space indentation) within lists for proper nesting. The current fenced blocks work, but converting them to indented style would align with Markdown best practices for list content.
Also applies to: 396-419
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/data-sources/builtin-toolsets/gcp.md
🧰 Additional context used
📓 Path-based instructions (1)
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
When writing documentation in the docs/ directory, always add a blank line between a header/bold text and a list, otherwise MkDocs won't render the list properly
Files:
docs/data-sources/builtin-toolsets/gcp.md
🪛 LanguageTool
docs/data-sources/builtin-toolsets/gcp.md
[grammar] ~7-~7: Ensure spelling is correct
Context: ...ed resources. ## Overview The GCP MCP addon consists of three specialized servers: ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
docs/data-sources/builtin-toolsets/gcp.md
341-341: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
396-396: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
451-451: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
451-451: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
457-457: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
457-457: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
463-463: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
463-463: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
469-469: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
469-469: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
475-475: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
475-475: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
483-483: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
495-495: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
502-502: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: llm_evals
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
- GitHub Check: build (3.11)
✅ Results of HolmesGPT evalsAutomatically triggered by commit 8919b19 on branch Results of HolmesGPT evals
Time/Cost columns show % change vs historical average (↑slower/costlier, ↓faster/cheaper). Changes under 10% shown as ±0%. Historical Comparison DetailsFilter: excluding branch 'gcloud-mcp' Status: Success - 23 test/model combinations loaded Experiments compared (30):
Comparison indicators:
📖 Legend
🔄 Re-run evals manually
Option 1: Comment on this PR with Or with more options (one per line): Run evals on a different branch (e.g., master) for comparison:
Quick re-run: Use Option 2: Trigger via GitHub Actions UI → "Run workflow" 🏷️ Valid markers
Commands: |
✅ Results of HolmesGPT evalsAutomatically triggered by commit 641743f on branch Results of HolmesGPT evals
Time/Cost columns show % change vs historical average (↑slower/costlier, ↓faster/cheaper). Changes under 10% shown as ±0%. Historical Comparison DetailsFilter: excluding branch 'gcloud-mcp' Status: Success - 23 test/model combinations loaded Experiments compared (30):
Comparison indicators:
📖 Legend
🔄 Re-run evals manually
Option 1: Comment on this PR with Or with more options (one per line): Run evals on a different branch (e.g., master) for comparison:
Quick re-run: Use Option 2: Trigger via GitHub Actions UI → "Run workflow" 🏷️ Valid markers
Commands: |
✅ Results of HolmesGPT evalsAutomatically triggered by commit 382e479 on branch Results of HolmesGPT evals
Time/Cost columns show % change vs historical average (↑slower/costlier, ↓faster/cheaper). Changes under 10% shown as ±0%. Historical Comparison DetailsFilter: excluding branch 'gcloud-mcp' Status: Success - 23 test/model combinations loaded Experiments compared (30):
Comparison indicators:
📖 Legend
🔄 Re-run evals manually
Option 1: Comment on this PR with Or with more options (one per line): Run evals on a different branch (e.g., master) for comparison:
Quick re-run: Use Option 2: Trigger via GitHub Actions UI → "Run workflow" 🏷️ Valid markers
Commands: |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
helm/holmes/templates/mcp-servers/gcp/_helpers.tplhelm/holmes/values.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- helm/holmes/templates/mcp-servers/gcp/_helpers.tpl
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-29T08:35:37.678Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T08:35:37.678Z
Learning: Applies to tests/llm/**/*.yaml : Never use `:latest` container tags in test infrastructure - use specific versions like `grafana/grafana:12.3.1`
Applied to files:
helm/holmes/values.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build (3.12)
- GitHub Check: llm_evals
- GitHub Check: build (3.11)
- GitHub Check: build (3.10)
✅ Results of HolmesGPT evalsAutomatically triggered by commit ec3e66a on branch Results of HolmesGPT evals
Time/Cost columns show % change vs historical average (↑slower/costlier, ↓faster/cheaper). Changes under 10% shown as ±0%. Historical Comparison DetailsFilter: excluding branch 'gcloud-mcp' Status: Success - 23 test/model combinations loaded Experiments compared (30):
Comparison indicators:
📖 Legend
🔄 Re-run evals manually
Option 1: Comment on this PR with Or with more options (one per line): Run evals on a different branch (e.g., master) for comparison:
Quick re-run: Use Option 2: Trigger via GitHub Actions UI → "Run workflow" 🏷️ Valid markers
Commands: |
Avi-Robusta
left a comment
Avi-Robusta
left a comment
There was a problem hiding this comment.
Looks good
The docs seem unclear, left a comment there.
Weird edge case, a user could apply this config which will make a deployment with no containers which i think would crash helm installations.
Not sure if we want to handle it or not, there is no reason for a user to do this
holmes:
mcpAddons:
gcp:
enabled: true
...
gcloud:
enabled: false
observability:
enabled: false
storage:
enabled: false
📂 Previous Runs📜 Run @ 07db59e (#20717205995)✅ Results of HolmesGPT evalsAutomatically triggered by commit 07db59e on branch Results of HolmesGPT evals
Time/Cost columns show % change vs historical average (↑slower/costlier, ↓faster/cheaper). Changes under 10% shown as ±0%. Historical Comparison DetailsFilter: excluding branch 'gcloud-mcp' Status: Success - 15 test/model combinations loaded Experiments compared (30):
Comparison indicators:
✅ Results of HolmesGPT evalsAutomatically triggered by commit 2dc1b97 on branch Results of HolmesGPT evals
Time/Cost columns show % change vs historical average (↑slower/costlier, ↓faster/cheaper). Changes under 10% shown as ±0%. Historical Comparison DetailsFilter: excluding branch 'gcloud-mcp' Status: Success - 26 test/model combinations loaded Experiments compared (30):
Comparison indicators:
📖 Legend
🔄 Re-run evals manually
Option 1: Comment on this PR with Or with more options (one per line): Run evals on a different branch (e.g., master) for comparison:
Quick re-run: Use Option 2: Trigger via GitHub Actions UI → "Run workflow" 🏷️ Valid markers
Commands: CLI: |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
docs/data-sources/builtin-toolsets/gcp.md (2)
370-382: Add blank lines between bold text and list items (MkDocs rendering requirement).Per the coding guidelines, bold text must be followed by a blank line before any list items for proper MkDocs rendering. Currently missing blank lines at:
- Line 370:
**What's Included:**→ needs blank line before line 371's bullet- Line 378:
**Security Boundaries:**→ needs blank line before line 379's bullet🔎 Proposed fix
**What's Included:** + - ✅ Complete audit log visibility (who changed what) - ✅ Full networking troubleshooting (firewalls, load balancers, SSL) - ✅ Database and BigQuery metadata (schemas, configurations) - ✅ Security findings and IAM analysis - ✅ Container and Kubernetes visibility - ✅ Monitoring, logging, and tracing **Security Boundaries:** + - ❌ NO actual data access (cannot read storage objects or BigQuery data) - ❌ NO secret values (only metadata) - ❌ NO write permissionsBased on coding guidelines: Add blank line between header/bold text and a list in MkDocs documentation files, otherwise lists won't render properly.
453-481: Add blank lines between subheaders and code blocks in Example Usage section.The coding guideline requires blank lines between headers and lists/content blocks for proper MkDocs rendering. This is missing for all five example subsections (lines 453, 459, 465, 471, 477). Additionally, the example code blocks should specify a language identifier (use
textfor plain-text examples).🔎 Proposed fix
### Investigating Deleted Pod Logs + - ``` + ```text "Show me logs from the payment-service pod that was OOMKilled this morning"Cross-Project Resource Discovery
"List all GKE clusters across our dev, staging, and prod projects"Audit Trail Investigation
"Who modified the firewall rules in the last 24 hours?"Storage Access Issues
"Why is my application getting 403 errors accessing the data-bucket?"SSL Certificate Problems
"Check the SSL certificates on our load balancers"</details> Based on coding guidelines: Add blank line between header/bold text and a list in MkDocs documentation files, otherwise lists won't render properly. </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (1)</summary><blockquote> <details> <summary>helm/holmes/values.yaml (1)</summary><blockquote> `219-248`: **Consider adding CPU limits and documenting memory limit variation.** The GCP MCP services have memory limits but no CPU limits, unlike the AWS MCP addon (lines 98-104) which sets both. Additionally, gcloud has a 1Gi memory limit while observability and storage have 512Mi, without explanation. <details> <summary>Considerations</summary> 1. **CPU limits**: Adding CPU limits prevents unbounded CPU usage: - Provides predictable resource consumption - Prevents noisy neighbor issues - Aligns with AWS MCP addon pattern (line 104: `cpu: "500m"`) 2. **Memory variation**: The 2x memory allocation for gcloud (1Gi vs 512Mi) might be intentional due to CLI overhead, but a comment would clarify the reasoning. Example adjustment: ```yaml gcloud: enabled: true image: "gcloud-cli-mcp:1.0.7" port: 8000 resources: requests: memory: "256Mi" cpu: "100m" limits: memory: "1Gi" # Higher limit due to gcloud CLI overhead cpu: "500m"
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/data-sources/builtin-toolsets/gcp.mdhelm/holmes/values.yaml
🧰 Additional context used
📓 Path-based instructions (1)
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Add blank line between header/bold text and a list in MkDocs documentation files, otherwise lists won't render properly
Files:
docs/data-sources/builtin-toolsets/gcp.md
🧠 Learnings (1)
📚 Learning: 2026-01-05T11:14:20.208Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.208Z
Learning: Applies to docs/**/*.md : Add blank line between header/bold text and a list in MkDocs documentation files, otherwise lists won't render properly
Applied to files:
docs/data-sources/builtin-toolsets/gcp.md
🪛 LanguageTool
docs/data-sources/builtin-toolsets/gcp.md
[grammar] ~10-~10: Ensure spelling is correct
Context: ...on for setup instructions. The GCP MCP addon consists of three specialized servers: ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
docs/data-sources/builtin-toolsets/gcp.md
344-344: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
399-399: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
454-454: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
454-454: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
460-460: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
460-460: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
466-466: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
466-466: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
472-472: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
472-472: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
478-478: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
478-478: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
486-486: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
498-498: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
505-505: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build
- GitHub Check: llm_evals
- GitHub Check: build (3.11)
- GitHub Check: build (3.10)
- GitHub Check: build (3.12)
🔇 Additional comments (1)
helm/holmes/values.yaml (1)
254-259: GCP llmInstructions map structure is correctly handled by Helm templates.The GCP addon's map structure for
llmInstructions(withgcloud,observability,storagekeys) is properly referenced in the templates. The helper functions inhelm/holmes/templates/mcp-servers/gcp/_helpers.tplcorrectly check and use.Values.mcpAddons.gcp.llmInstructions.gcloud,.Values.mcpAddons.gcp.llmInstructions.observability, and.Values.mcpAddons.gcp.llmInstructions.storagewith fallback defaults, ensuring proper template rendering regardless of whether custom instructions are provided.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
helm/holmes/values.yaml (1)
213-249: Individual service configurations are well-structured.Each GCP MCP service (gcloud, observability, storage) has dedicated resources, ports, and images. The higher memory limit for gcloud (1Gi vs 512Mi) is appropriate given it's the general-purpose CLI tool.
Note that all three services default to
enabled: true. While this provides full functionality out of the box, users may want to disable services they don't need to conserve cluster resources. Consider if this is the desired behavior or if a more conservative default (e.g., only enabling gcloud) would be better.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/data-sources/builtin-toolsets/.nav.ymlhelm/holmes/templates/toolset-config.yamlhelm/holmes/values.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
helm/holmes/templates/toolset-config.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: llm_evals
- GitHub Check: build (3.11)
- GitHub Check: build (3.10)
- GitHub Check: build (3.12)
🔇 Additional comments (7)
helm/holmes/templates/toolset-config.yaml (2)
1-1: LGTM! Condition properly extended for GCP addon.The top-level condition correctly includes GCP MCP addon, maintaining consistency with AWS, MariaDB, and Azure patterns.
48-84: GCP MCP server configuration is well-structured and correct.Port numbers are properly configured in values.yaml (gcloud: 8000, observability: 8001, storage: 8002) and correctly referenced in the template with explicit
intcasting for type safety. The SSE mode is intentional and consistent with the/sseendpoint paths used by the GCP MCP servers. Each service is properly guarded by individual enable flags, and the pattern matches the established implementation for other MCP addons.docs/data-sources/builtin-toolsets/.nav.yml (1)
2-37: LGTM! GCP navigation entry added correctly.The GCP (MCP) navigation entry is properly positioned in alphabetical order, and the indentation has been standardized to 2 spaces for consistency. The referenced
gcp.mdfile issue from the previous review has been addressed.helm/holmes/values.yaml (4)
162-181: Excellent documentation for GCP MCP setup.The authentication and multi-project support sections provide clear, actionable instructions with concrete commands. This will significantly improve the user experience for configuring the GCP MCP addon.
182-206: Core configuration is well-structured and secure.The configuration properly defaults to
enabled: falsefor safe deployment, andnetworkPolicy.enabled: truefor security. The note about Workload Identity not working with gcloud CLI (line 200-202) is valuable context for users.Previous review comments regarding
imagePullPolicyand placeholder URLs have been properly addressed.
207-212: LGTM! Config section provides appropriate defaults and flexibility.The configuration allows for dynamic project/region selection via flags while providing sensible defaults. The comments clearly explain when each field is needed.
250-256: LGTM! Per-service LLM instruction customization provides good flexibility.The ability to customize instructions for each service (gcloud, observability, storage) while defaulting to built-in instructions is well-designed. This aligns with the template's helper function usage.
4 participants
Add support for GCP mcp servers
include:
Summary by CodeRabbit
New Features
Enhancements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.