Refresh the ISO URL when returning it#31
Conversation
The download URL in the infraenv will be valid for a few hours after the cluster is created, but once the URL expires the dedicated URL API is required to refresh it. This commit updates the cluster_iso_download_url tool to just use that API all the time instead of pulling the url and expiration out of the infraenv.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: carbonin The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughThe changes introduce a helper function for formatting presigned URLs and refactor related tools to use this function and a new client method that returns a typed Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Server
participant InventoryClient
participant AssistedServiceAPI
User->>Server: Request ISO download URL
Server->>InventoryClient: get_infra_env_download_url(infra_env_id)
InventoryClient->>AssistedServiceAPI: get_infra_env_download_url(infra_env_id)
AssistedServiceAPI-->>InventoryClient: PresignedUrl object
InventoryClient-->>Server: PresignedUrl object
Server->>Server: format_presigned_url(PresignedUrl)
Server-->>User: Formatted URL string
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
openshift-ci
Bot
added
the
size/L
![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
🧹 Nitpick comments (4)
service_client/assisted_service_api.py (1)
688-716: InstantiateInstallerApionly once per call chain
asyncio.to_thread()is passedself._installer_api().get_infra_env_download_url, which recreates anApiClient/InstallerApievery time this helper is invoked.
Because many higher-level flows iterate over several infra-envs (seecluster_iso_download_urlin server.py), this can result in a noticeable amount of unnecessary client construction and TLS handshakes.- result = await asyncio.to_thread( - self._installer_api().get_infra_env_download_url, - infra_env_id=infra_env_id, - ) + api_client = self._installer_api() + result = await asyncio.to_thread( + api_client.get_infra_env_download_url, + infra_env_id=infra_env_id, + )Caching the
InstallerApi(or the underlyingApiClient) for the lifetime ofInventoryClient, or at least re-using it within the same coroutine, would cut the overhead without changing observable behaviour.server.py (2)
21-40: Edge-case: missing or blank URL
format_presigned_urlblindly interpolatespresigned_url.url, which is allowed to beNone/empty by the OpenAPI model.
If the API ever returns an object with no URL (e.g. quota exceeded), the string will contain “URL: None”, which is misleading.- response_parts = [f"URL: {presigned_url.url}"] + if not presigned_url.url: + return "No URL returned by the service." + response_parts = [f"URL: {presigned_url.url}"]Consider short-circuiting or raising to make the failure explicit.
253-263: Sequential I/O could be parallelisedFor clusters with many infra-envs the loop performs one API call at a time:
for infra_env in infra_envs: presigned_url = await client.get_infra_env_download_url(infra_env_id)Those calls are network-bound and independent, so gathering them concurrently can cut total latency:
tasks = [ client.get_infra_env_download_url(env["id"]) for env in infra_envs ] for presigned_url in await asyncio.gather(*tasks): ...Not critical for small clusters but worth considering.
tests/test_server.py (1)
362-376: Minor: keep test data consistentThe mocked infra-env id is
"test-infraenv-id"but the URL embedded in the expectation uses"test-id". Tests pass because the function just echoes the mocked URL, yet inconsistent fixtures make debugging harder. Align the id in the URL (or derive it from the fixture) to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
server.py(3 hunks)service_client/assisted_service_api.py(1 hunks)tests/test_assisted_service_api.py(1 hunks)tests/test_server.py(9 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
server.py (1)
service_client/assisted_service_api.py (1)
get_infra_env_download_url(675-716)
tests/test_server.py (2)
service_client/assisted_service_api.py (1)
get_infra_env_download_url(675-716)tests/test_utils.py (1)
create_test_presigned_url(82-90)
tests/test_assisted_service_api.py (2)
service_client/assisted_service_api.py (1)
get_infra_env_download_url(675-716)tests/test_utils.py (1)
create_test_presigned_url(82-90)
🔇 Additional comments (1)
tests/test_assisted_service_api.py (1)
690-771: Solid coverage👍 Comprehensive positive / negative cases for the new API; mocks are isolated and assertions are precise.
|
/retest |
2 participants
The download URL in the infraenv will be valid for a few hours after the cluster is created, but once the URL expires the dedicated URL API is required to refresh it. This commit updates the
cluster_iso_download_urltool to just use that API all the time instead of pulling the url and expiration out of the infraenv.Summary by CodeRabbit
New Features
Refactor
Tests