Pass sleep_time as argument in wait_for_condition method #2270
Conversation
Signed-off-by: Geetika Kapoor <[email protected]>
WalkthroughThe pull request introduces a modification to the Changes
Finishing Touches
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
ocp_resources/resource.py (1)
Test coverage needed for the
sleep_timeparameterThe
wait_for_conditionmethod is tested intests/test_resources.py, but there are no explicit test cases verifying the behavior with differentsleep_timevalues. Consider adding test cases to verify:
- Custom sleep intervals work as expected
- Edge cases (e.g., very small or large sleep times)
🔗 Analysis chain
Line range hint
1030-1062: Verify the usage of the new parameter in tests.The implementation looks correct, but we should verify that tests are updated to cover the new parameter.
Run the following script to check for test coverage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check test coverage for wait_for_condition method # Look for test files that use wait_for_condition rg -l "wait_for_condition" test_*.py # Look for specific test cases that verify the sleep_time parameter rg "wait_for_condition.*sleep_time" test_*.pyLength of output: 188
Script:
#!/bin/bash # First, find all test files in the repository echo "=== Finding test files ===" fd "test_.+\.py$" # Then search for wait_for_condition usage in the entire repo echo -e "\n=== Searching for wait_for_condition usage ===" rg -A 2 "wait_for_condition" # Look for specific test cases with sleep_time parameter echo -e "\n=== Searching for sleep_time parameter usage ===" rg "wait_for_condition.*sleep_time"Length of output: 1981
🧹 Nitpick comments (1)
ocp_resources/resource.py (1)
Line range hint
1030-1062: Consider renaming parameter and adding validation.The implementation looks good and achieves the PR objective. However, consider these improvements:
- Rename
sleep_timetopolling_intervalto better indicate its purpose- Add validation for non-positive values that could cause issues
Apply this diff to improve the implementation:
-def wait_for_condition(self, condition: str, status: str, timeout: int = 300, sleep_time: int = 1) -> None: +def wait_for_condition(self, condition: str, status: str, timeout: int = 300, polling_interval: int = 1) -> None: """ Wait for Resource condition to be in desire status. Args: condition (str): Condition to query. status (str): Expected condition status. timeout (int): Time to wait for the resource. - sleep_time(int): Interval between each retry when checking the resource's condition. + polling_interval (int): Interval in seconds between condition checks. Raises: TimeoutExpiredError: If Resource condition in not in desire status. + ValueError: If polling_interval is less than or equal to 0. """ + if polling_interval <= 0: + raise ValueError("polling_interval must be greater than 0") + self.logger.info(f"Wait for {self.kind}/{self.name}'s '{condition}' condition to be '{status}'") timeout_watcher = TimeoutWatch(timeout=timeout) for sample in TimeoutSampler( wait_timeout=timeout, - sleep=sleep_time, + sleep=polling_interval, func=lambda: self.exists, ): if sample: break for sample in TimeoutSampler( wait_timeout=timeout_watcher.remaining_time(), - sleep=sleep_time, + sleep=polling_interval, func=lambda: self.instance, ):
|
/verified |
Short description:
Add sleep_time as argument. Resources like pipeline,pipeline runs need more time and polling in '1s' is not needed.
More details:
What this PR does / why we need it:
Which issue(s) this PR fixes:
Special notes for reviewer:
Bug:
Summary by CodeRabbit
wait_for_conditionmethod with configurable sleep interval between retry attemptssleep_timeparameter to control wait duration during resource condition checks