Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add info msg to cli output if logs are not available #1689

Merged
merged 1 commit into from
Jan 16, 2025

Conversation

BelSasha
Copy link
Contributor

No description provided.

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@BelSasha BelSasha marked this pull request as ready for review January 16, 2025 11:13
runhouse/main.py Outdated
@@ -501,8 +501,14 @@ def cluster_logs(
headers=rns_client.request_headers(),
)

if (
resp.status_code == 404
and resp.json().get("detail") == "No logs found for the requested resource"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to parse the output, don't we know if its a 404 that the logs arent readfy yety?

@BelSasha BelSasha force-pushed the sb/info-msg-for-no-logs branch from 34dd406 to 42ece33 Compare January 16, 2025 15:19
@BelSasha BelSasha requested a review from jlewitt1 January 16, 2025 15:20
runhouse/main.py Outdated
if resp.status_code != 200:
console.print("Failed to load cluster logs.")
console.print(f"Failed to load cluster logs: {resp.json().get('detail')}.")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any additional info here in the response we would want to expose to a user?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well it feels like it will be a bit easier to debug it, since the user might just report us the response msg back to us so well get a sense of what went wrong before going though the logs. But I see your point why we should not print it to the user, removing.

@BelSasha BelSasha force-pushed the sb/info-msg-for-no-logs branch from 42ece33 to 27c8405 Compare January 16, 2025 16:24
@BelSasha BelSasha requested a review from jlewitt1 January 16, 2025 16:24
@BelSasha BelSasha merged commit 702445a into main Jan 16, 2025
11 checks passed
@BelSasha BelSasha deleted the sb/info-msg-for-no-logs branch January 16, 2025 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants