Add explicit expiration time to iso download tool#28
Add explicit expiration time to iso download tool#28carbonin merged 1 commit intoopenshift-assisted:masterfrom
Conversation
Previously the model was showing inconsistent behavior regarding the expiration time. Sometimes it would extract it from the claims in the url, sometimes it would print it in a readable format, and sometimes it wouldn't print anything at all. This should make it more consistent.
|
[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 update the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant InfraEnvDB
Client->>Server: Request ISO download URLs for cluster_id
Server->>InfraEnvDB: Query infraenvs for cluster_id
InfraEnvDB-->>Server: Return infraenv data (URLs, expires_at)
Server->>Server: Format output string (URL, optional Expires at)
Server-->>Client: Return formatted ISO URLs with expiration info
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/M
![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 (1)
README.md (1)
112-113: Minor: Consider consistent markdown list style.The static analysis tool suggests using asterisks instead of dashes for consistency with the rest of the documentation.
- - URL: <download-url> - - Expires at: <expiration-timestamp> (if available) + * URL: <download-url> + * Expires at: <expiration-timestamp> (if available)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)server.py(2 hunks)tests/test_server.py(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/test_server.py (2)
service_client/assisted_service_api.py (1)
list_infra_envs(256-291)server.py (1)
cluster_iso_download_url(203-260)
🪛 markdownlint-cli2 (0.17.2)
README.md
112-112: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
113-113: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
⏰ 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). (1)
- GitHub Check: Red Hat Konflux / assisted-service-mcp-saas-main-on-pull-request
🔇 Additional comments (6)
server.py (2)
211-215: LGTM: Documentation accurately reflects the new structured format.The updated docstring clearly describes the new output format with explicit URL and expiration information.
231-260: LGTM: Well-implemented structured output with proper expiration handling.The implementation correctly:
- Extracts both URL and expiration data from infra environments
- Formats the response with clear prefixes
- Filters out zero/default expiration dates consistently with
cluster_credentials_download_url- Separates multiple ISOs with blank lines for readability
The logic handles edge cases appropriately and maintains consistency with the existing codebase.
README.md (1)
111-114: LGTM: Documentation accurately reflects the new implementation.The updated documentation clearly describes the structured output format with URL and optional expiration information, matching the implementation in
server.py.tests/test_server.py (3)
237-237: LGTM: Appropriate pylint disable for test class.The disable comment is correctly placed for the test class which naturally has many public test methods.
361-361: LGTM: Tests properly updated to match new structured format.The test expectations correctly reflect the new output format with "URL:" and "Expires at:" prefixes, and proper separation of multiple ISOs with blank lines.
Also applies to: 370-370, 390-390, 400-400, 414-417
422-475: LGTM: Comprehensive edge case testing.The new test cases properly cover important edge cases:
test_cluster_iso_download_url_no_expiration: Validates behavior whenexpires_atis missingtest_cluster_iso_download_url_zero_expiration: Validates behavior whenexpires_atis a zero/default dateBoth tests correctly expect only the URL line without expiration information, matching the implementation logic.
|
/lgtm |
|
@oourfali: changing LGTM is restricted to collaborators DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
2 participants
Previously the model was showing inconsistent behavior regarding the expiration time. Sometimes it would extract it from the claims in the url, sometimes it would print it in a readable format, and sometimes it wouldn't print anything at all. This should make it more consistent.
Summary by CodeRabbit