Repository navigation
Service Now - Adds health check table configuration - #1537
Conversation
Allows users to configure the table used for the ServiceNow health check. This change enables users to specify a custom table for the health check to accommodate API keys with limited permissions, improving the tool's adaptability. It also includes error responses for debugging. Signed-off-by: Tomer Keshet <tomer@robusta.dev>
|
Sheeproid
changed the title
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📂 Previous Runs📜 Run @ 43511e9 (#21863182373)✅ Results of HolmesGPT evalsAutomatically triggered by commit 43511e9 on branch Results of HolmesGPT evals
✅ Results of HolmesGPT evalsAutomatically triggered by commit e4f8307 on branch Results of HolmesGPT evals
📖 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: |
|
✅ 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:12beaa5
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:12beaa5 me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:12beaa5
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:12beaa5Patch 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:12beaa5Robusta 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:12beaa5 |
WalkthroughAdds two optional ServiceNow config fields in docs and a configurable Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Tip We've launched Issue Planner and it is currently in beta. Please try it out and share your feedback on Discord! 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 |
🔬 CLI Performance Benchmark🟡 Startup Time (no LLM)Measures
🟡 Full CLI with LLMMeasures
PR: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@holmes/plugins/toolsets/servicenow_tables/servicenow_tables.py`:
- Line 91: The _perform_health_check method is missing a type annotation for its
table_name parameter; update the function signature of
_perform_health_check(self, table_name) to include the type hint (table_name:
str) so it reads _perform_health_check(self, table_name: str) -> Tuple[bool,
str], ensuring imports for Tuple remain valid and mypy/type-checking passes.
- Around line 118-122: The returned error message in the except block catching
requests.exceptions.ConnectionError contains an accidental double space before
"Full error" — update the f-string in the except clause (the return that returns
(False, f"...")) to remove the extra space so the sentence reads "...unknown).
Full error: {str(e)}" (adjust the f-string around self.config.api_url and str(e)
accordingly).
🧹 Nitpick comments (2)
holmes/plugins/toolsets/servicenow_tables/servicenow_tables.py (2)
93-97: Stale comment: still referencessys_db_objectbut table is now dynamic.The comment on line 94 says "Query sys_db_object table" but the table is now configurable via the
table_nameparameter.Proposed fix
- # Query sys_db_object table with minimal data + # Query the specified table with minimal data to verify connectivity
100-100: Consider using!sconversion flag instead ofstr()in f-strings.Ruff (RUF010) flags
f"...{str(e)}"— the idiomatic form isf"...{e!s}". This applies to lines 121 (and line 89, though outside this range). The success message on line 100 is also flagged by TRY300 suggesting it move to anelseblock, though that's stylistic.Proposed fix (line 121)
- f"Failed to connect to ServiceNow instance at {self.config.api_url if self.config else 'unknown'}. Full error: {str(e)}", + f"Failed to connect to ServiceNow instance at {self.config.api_url if self.config else 'unknown'}. Full error: {e!s}",Also applies to: 106-106, 111-111, 121-121
…erformanceMetrics teardown TestTransformerPerformanceMetrics.teardown_method was not restoring the original llm_summarize transformer after replacing it with a mock. When pytest-xdist scheduled test_yaml_transformer_parsing on the same worker after this class, the registry was missing llm_summarize, causing the YAML parser to silently drop transformer configs. Signed-off-by: Tomer Keshet <tomer@robusta.dev>
Signed-off-by: Tomer Keshet <tomer@robusta.dev> Signed-off-by: Mohse Morad <moshemorad12340@gmail.com>
Signed-off-by: Tomer Keshet <tomer@robusta.dev> Signed-off-by: Mohse Morad <moshemorad12340@gmail.com>
2 participants
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests