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 timeout on matrix connection and related errors handling #1984

Conversation

flefebvre03
Copy link

@flefebvre03 flefebvre03 commented Feb 23, 2023

Description

Fixes #1750

Status

READY

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • Attempted connection to non-existent matrix server, checked that an error is logged, and that no exception are raised

Checklist:

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Copy link
Member

@FabioRosado FabioRosado left a comment

Choose a reason for hiding this comment

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

Hello thank you for working on this fix and opening the PR!

I've added some comments but they are non blocking for this PR so I'm approving it

opsdroid/connector/matrix/connector.py Outdated Show resolved Hide resolved
opsdroid/connector/matrix/connector.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Feb 24, 2023

Codecov Report

Patch coverage: 33.33% and project coverage change: -0.08 ⚠️

Comparison is base (a44389a) 99.16% compared to head (edb461c) 99.09%.

❗ Current head edb461c differs from pull request most recent head 958bc5f. Consider uploading reports for the commit 958bc5f to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1984      +/-   ##
==========================================
- Coverage   99.16%   99.09%   -0.08%     
==========================================
  Files          83       83              
  Lines        5395     5400       +5     
==========================================
+ Hits         5350     5351       +1     
- Misses         45       49       +4     
Impacted Files Coverage Δ
opsdroid/connector/matrix/connector.py 97.36% <33.33%> (-1.15%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@flefebvre03
Copy link
Author

I believe it's ready to be merged.
Is there anything else I have to do?

Copy link
Contributor

@Cadair Cadair left a comment

Choose a reason for hiding this comment

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

I think this is incomplete, you don't seem to use the timeout or the retry?

opsdroid/connector/matrix/connector.py Show resolved Hide resolved
opsdroid/connector/matrix/connector.py Outdated Show resolved Hide resolved
flefebvre03 and others added 2 commits March 7, 2023 18:31
update error message

Co-authored-by: Stuart Mumford <stuart@cadair.com>
@jacobtomlinson
Copy link
Member

Sorry we never got this over the line. Given the age of this PR I'm going to close it out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

If the matrix server is unreachable on startup no error is logged
5 participants