feat: make api-client timeout configurable, increase in admin cli #1950
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR adds configurable timeout settings to the API client, with a specific focus on extending timeouts for admin CLI operations that require longer processing times.
- Added 120-second timeout in
/admin/src/client.rsfor admin operations that may exceed default limits - Modified
ShuttleApiClient::new()in/api-client/src/lib.rsto accept an optional timeout parameter, defaulting to 60 seconds - Implemented timeout configuration through
reqwest::Client::builder().timeout()for consistent HTTP client behavior
3 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
| .build() | ||
| .unwrap(); |
There was a problem hiding this comment.
logic: unwrap() could panic if client builder fails. Consider using ? operator or proper error handling here
There was a problem hiding this comment.
PR Summary
(updates since last review)
This PR extends the timeout configuration implementation by adding CLI argument support and runtime integration.
- Added
client_timeoutCLI argument in/admin/src/args.rswith 120-second default - Updated runtime initialization in
/runtime/src/rt.rsto handle optional timeout parameter - Modified admin client constructor in
/admin/src/lib.rsto pass through CLI timeout value
4 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
There was a problem hiding this comment.
PR Summary
(updates since last review)
This PR continues to refine the API client timeout implementation with a focus on the admin CLI integration. Here's a summary of the latest changes:
- Added documentation in
/admin/src/args.rsto clarify thatclient_timeoutis specified in seconds - Integrated timeout configuration in
/admin/src/lib.rsthrough theClient::new()constructor - Implemented timeout handling in
/api-client/src/lib.rsviareqwest::Client::builder().timeout()
The changes are focused and maintain consistency with the previous implementation while improving clarity around timeout units and configuration.
1 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
| #[arg(long, default_value_t = 120)] | ||
| pub client_timeout: u64, |
There was a problem hiding this comment.
style: Consider adding validation to prevent 0 or negative timeout values
| #[arg(long, default_value_t = 120)] | ||
| pub client_timeout: u64, |
There was a problem hiding this comment.
style: 120 seconds may be too short for resource-intensive operations like certificate renewals or mass GC operations
Description of change
Admin CLI requires longer timeout for certain operations.