-
Notifications
You must be signed in to change notification settings - Fork 1
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
Do not crash Syncsketch addon if it cannot connect to the Syncsketch server due to wrong server or credentials #9
Conversation
…server due to wrong server or credentials
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
2024-10-25 12:50:45 ERROR server Error during syncsketch 0.2.0+dev setup
2024-10-25 12:50:45 ERROR server Addon syncsketch 0.2.0+dev failed to start. Unloading. |
Thanks @MilaKudr could you try again? |
@martastain you're way more knowledgable here - should there be better ways to catch or log these |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to handle TimeoutError at least.
I am not sure what status codes we should expect - but raising ValueError during setup here:
if webhook_created.status_code != 200:
logging.info(
"Something went wrong when trying to create the Webhook.")
raise ValueError(webhook_created.json())
Will disable the addon, while server unavailable will keep it alive, but probably defunct as it just returns.
BTW: it does not hurt in this case because it happens during server initialization, but it is not a good idea to use requests (blocking) in async functions. Use httpx instead.
@martastain could you please convert it to use |
I've replaced requests.request calls with httpx, could you pls test the changes? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So tested and works like a charm! Thank you @BigRoy.
PS: it was not easy to test since docker is quite bithing about the new #.#.#+dev
tagging in version. The previouse #.#.#-dev.1
was more afficient for sure @dee-ynput.
Changelog Description
Do not crash Syncsketch addon if it cannot connect to the Syncsketch server due to wrong server or credentials
Additional info
Previously this would error with:
Testing notes: