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

Don't show server response in truss push errors #721

Merged
merged 2 commits into from
Nov 8, 2023

Conversation

helenlyang
Copy link
Contributor

@helenlyang helenlyang commented Nov 7, 2023

Summary

This PR removes the full server response from the Truss CLI console output on truss push errors. The console output will only include the message used to initialize the ApiError.

IIUC this is the only usage of ApiError; here, the ApiError message is set to the message of the first error in the server response.

Testing

I manually added a second remote to ~/.trussrc with an invalid API key:

[baseten]
remote_provider = baseten
api_key = {MY_ACTUAL_API_KEY}
remote_url = https://app.baseten.co
[baseten-invalid]
remote_provider = baseten
api_key = abc
remote_url = https://app.baseten.co

Running poetry run truss push and selecting the invalid remote with this change:

? 🎮 Which remote do you want to connect to? baseten-invalid
You have to log in to perform the request

Prior to this change:

? 🎮 Which remote do you want to connect to? baseten-invalid
You have to log in to perform the request
<Server response: {'errors': [{'message': 'You have to log in to perform the request', 'locations': [{'line': 3, 'column': 13}], 'path': ['models'], 'extensions': {'code': 'UNAUTHENTICATED_ACCESS'}}], 'data': {'models': None}}>

Copy link

linear bot commented Nov 7, 2023

BT-9122 Format truss errors better

In the Truss CLI, let's stop showing errors like this on push:

$ truss push
? 🎮 Which remote do you want to connect to? sid-staging
The user does not have the authorization to perform the request
<Server response: {'errors': [{'message': 'The user does not have the authorization to perform the request', 'locations': [{'line': 3, 'column': 13}], 'path': ['models'], 'extensions': {'code': 'UNAUTHORIZED_ACCESS'}}], 'data': {'models': None}}>

We should instead have a clearer error without the "Server response:" piece. Just remove that print for now

@helenlyang helenlyang requested a review from squidarth November 7, 2023 18:54
@helenlyang helenlyang changed the title Don't show server response in truss push errors Don't show server response in truss push errors Nov 7, 2023
@helenlyang helenlyang marked this pull request as ready for review November 7, 2023 19:48
@@ -17,16 +14,7 @@ def __init__(self, message, response=None):
self.response = response
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can remove all of this response stuff now and we can just pass for the whole class

@helenlyang helenlyang requested a review from squidarth November 8, 2023 19:23
@helenlyang helenlyang merged commit 9045cee into main Nov 8, 2023
3 checks passed
@helenlyang helenlyang deleted the helenyang/bt-9122-format-truss-errors-better branch November 8, 2023 22:57
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