Timeout limit for HTTP notifications #48
Conversation
WalkthroughThe changes made in this pull request involve the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/appier_extras/parts/admin/models/event.py (2)
185-185: Document the timeout parameter.Consider adding a comment to document the expected format and units of the timeout parameter (e.g., seconds, milliseconds).
185-189: Consider adding timeout validation and defaults.The implementation looks good, but could be enhanced with:
- Type validation for the timeout parameter
- A reasonable default timeout value
- Value range validation to prevent negative timeouts
Consider applying this improvement:
- timeout = arguments.get("timeout", None) + timeout = arguments.get("timeout", 30.0) # Default 30 seconds timeout + if timeout is not None: + try: + timeout = float(timeout) + if timeout <= 0: + raise ValueError("Timeout must be positive") + except (TypeError, ValueError): + raise ValueError("Invalid timeout value: must be a positive number")
| timeout = arguments.get("timeout", None) | ||
| retries = arguments.get("retries", 3) | ||
| logger = appier.get_logger() | ||
| logger.debug("Running HTTP notification for '%s' ..." % url) | ||
| return cls._retry(lambda: appier.post(url, data_j=arguments), count=retries) | ||
| return cls._retry(lambda: appier.post(url, timeout=timeout, data_j=arguments), count=retries) |
There was a problem hiding this comment.
💡 Codebase verification
Add test coverage for HTTP notification timeout parameter
The timeout parameter lacks test coverage and documentation:
- No test cases found for HTTP notification timeout scenarios in
src/appier_extras/test/admin/models/event.py - No documentation found describing the timeout configuration in the documentation files
- The feature is only implemented in the
notify_httpmethod without corresponding test coverage
Please add:
- Test cases covering timeout behavior
- Documentation for the timeout parameter in the HTTP notification configuration
- Example usage in the documentation
🔗 Analysis chain
Verify test coverage and documentation.
The timeout parameter addition should be reflected in:
- Unit tests covering timeout scenarios
- Documentation for HTTP notification configuration
- Example configurations demonstrating timeout usage
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for tests and documentation related to HTTP notifications
# Check for test files containing HTTP notification tests
echo "Searching for test files..."
fd -e py test | xargs rg -l "notify_http"
# Check for documentation files
echo "Searching for documentation files..."
fd -e md -e rst | xargs rg -l "notify_http"
Length of output: 279
Script:
#!/bin/bash
# Let's try a broader search to understand the HTTP notification implementation and testing
# Search for HTTP notification related code and tests
echo "Searching for HTTP notification patterns..."
rg -l "post.*timeout|timeout.*post|notify.*http" --type py
# Search for test files that might contain HTTP-related tests
echo "Searching for HTTP-related tests..."
rg -l "def test.*http|http.*test" --type py
# Look for configuration examples
echo "Searching for configuration examples..."
fd -e json -e yaml -e yml | xargs rg -l "notification|http|timeout"
# Check documentation more broadly
echo "Searching documentation more broadly..."
fd README -e md -e rst | xargs rg -l "notification|http|timeout"
Length of output: 5045
Script:
#!/bin/bash
# Let's examine the test file and documentation content specifically related to HTTP notifications
# Check the event model test file content
echo "Checking event model test content..."
rg -A 5 -B 5 "notify_http|post.*timeout" src/appier_extras/test/admin/models/event.py
# Check documentation content
echo "Checking documentation content..."
rg -A 5 -B 5 "notification|http.*timeout" doc/models.md doc/configuration.md README.md
# Look at the event model implementation
echo "Checking event model implementation..."
rg -A 5 -B 5 "notify_http" src/appier_extras/parts/admin/models/event.py
Length of output: 841
|
Hi @joamag ! Sorry to bother you! Please let me know if there is anything else I need to do to get this PR merged. Thank you! 🙇 |
Allow the timeout limit to be specified when sending events through HTTP.
This way, it is possible to avoid being impacted by the performance of external services when they are not ready to receive events.
Summary by CodeRabbit
New Features
timeoutparameter for improved request control.Bug Fixes
timeoutvalue from the arguments, ensuring correct execution timing.