-
Notifications
You must be signed in to change notification settings - Fork 292
add more logs and support for tcp keepalive #1992
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: master
Are you sure you want to change the base?
Conversation
|
✅ 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/robusta-runner:a789e8f
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/robusta-runner:a789e8f me-west1-docker.pkg.dev/robusta-development/development/robusta-runner-dev:a789e8f
docker push me-west1-docker.pkg.dev/robusta-development/development/robusta-runner-dev:a789e8fPatch Helm values in one line: helm upgrade --install robusta robusta/robusta \
--reuse-values \
--set runner.image=me-west1-docker.pkg.dev/robusta-development/development/robusta-runner-dev:a789e8f |
WalkthroughAdds four TCP keepalive environment constants, exposes them in env vars, applies keepalive socket options in the WebSocket receiver when enabled, adds debug logging around WebSocket ping/keepalive and action/stream handling, and updates Slack tests to fetch messages by timestamp. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (2)
✏️ Tip: You can disable this entire section by setting 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 |
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/utils/slack_utils.py`:
- Around line 26-36: The return type annotation of get_message_by_ts is
incorrect because it can return None; update the signature to return
Optional[str] and add the corresponding import from typing (e.g., from typing
import Optional), keeping the existing logic that uses
self.client.conversations_history and messages; ensure the annotation reads
Optional[str] for get_message_by_ts(self, ts: str) -> Optional[str].
🧹 Nitpick comments (2)
tests/test_slack.py (2)
26-27: Good improvement using timestamp-based retrieval for deterministic testing.This avoids race conditions when tests share a channel. Consider adding an explicit None check for clearer failure messages if the message isn't found:
ts = slack_sender.send_finding_to_slack(finding, slack_params, False) message = slack_channel.get_message_by_ts(ts) assert message is not None, f"Message with ts={ts} not found" assert message == msg
77-80: Consider updating other tests to useget_message_by_ts()for consistency.This test and others (
test_send_file_named_tempfile_fails,test_temporary_file_creation_failure) still useget_latest_message(). If the rationale for the change intest_send_to_slackapplies here, these could be updated similarly by capturing thetsreturn value. This would align with the guidance inslack_utils.pyto prefer timestamp-based retrieval.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/test_slack.pytests/utils/slack_utils.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_slack.py (2)
src/robusta/integrations/slack/sender.py (1)
send_finding_to_slack(660-682)tests/utils/slack_utils.py (1)
get_message_by_ts(26-36)
⏰ 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). (2)
- GitHub Check: run_tests
- GitHub Check: run_tests
🔇 Additional comments (1)
tests/utils/slack_utils.py (1)
20-24: Helpful comment for guiding users to the better API.The note clarifies when to prefer timestamp-based retrieval over latest message retrieval.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
No description provided.