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 support to login to the Matrix connector with an access_token #1707

Merged
merged 13 commits into from Feb 6, 2021

Conversation

Cadair
Copy link
Contributor

@Cadair Cadair commented Jan 16, 2021

Description

This adds support for configuring the matrix connector with an access_token.

Fixes #813

It also adds the first matrix connector tests using the framework in #1558 and #1706 (so it is based on #1706)

Status

*UNDER DEVELOPMENT

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

I need to test this IRL.

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

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

I will push something in an hour ish with my suggestions. my comments were for PR #1706. Changes pushed there.

opsdroid/testing/tests/test_testing.py Outdated Show resolved Hide resolved
opsdroid/connector/github/tests/test_connector_github.py Outdated Show resolved Hide resolved
opsdroid/connector/github/tests/test_connector_github.py Outdated Show resolved Hide resolved
opsdroid/connector/github/tests/test_connector_github.py Outdated Show resolved Hide resolved
opsdroid/connector/github/tests/test_connector_github.py Outdated Show resolved Hide resolved
opsdroid/connector/github/tests/test_connector_github.py Outdated Show resolved Hide resolved
opsdroid/conftest.py Outdated Show resolved Hide resolved
opsdroid/conftest.py Outdated Show resolved Hide resolved
opsdroid/testing/fixtures.py Outdated Show resolved Hide resolved
opsdroid/testing/fixtures.py Outdated Show resolved Hide resolved
@Cadair Cadair force-pushed the matrix_access_token branch 3 times, most recently from 0bc2259 to d36a0fc Compare January 17, 2021 13:27
@codecov
Copy link

codecov bot commented Jan 17, 2021

Codecov Report

Merging #1707 (77d3659) into master (ad8007b) will decrease coverage by 0.11%.
The diff coverage is 98.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1707      +/-   ##
==========================================
- Coverage   99.54%   99.42%   -0.12%     
==========================================
  Files          71       71              
  Lines        4176     4196      +20     
==========================================
+ Hits         4157     4172      +15     
- Misses         19       24       +5     
Impacted Files Coverage Δ
opsdroid/testing/external_api.py 98.90% <95.65%> (-1.10%) ⬇️
opsdroid/connector/matrix/connector.py 98.85% <100.00%> (-1.15%) ⬇️
opsdroid/core.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad8007b...d5b2820. Read the comment docs.

@Cadair
Copy link
Contributor Author

Cadair commented Jan 17, 2021

(lint fails here would be fixed by #1709 )

@Cadair Cadair marked this pull request as draft January 20, 2021 13:32
@Cadair Cadair marked this pull request as ready for review February 6, 2021 15:14
@Cadair
Copy link
Contributor Author

Cadair commented Feb 6, 2021

This now also adds support to the external api server to track multiple calls against the same route with different methods.

Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

Test infra changes LGTM

Copy link
Contributor

@SolarDrew SolarDrew left a comment

Choose a reason for hiding this comment

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

Nothing useful to add really, looks good 👍

status, response = self.responses[route].pop(0)
method = request.method

self._calls[(route, method)].append(request)
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume making this key a tuple instead of just route doesn't break a bunch of stuff downstream?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed all the things it broke 😀

@Cadair Cadair merged commit 2f9218c into opsdroid:master Feb 6, 2021
@Cadair Cadair deleted the matrix_access_token branch February 6, 2021 21:29
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.

support providing an access token in the configuration for the matrix connector rather than the password
3 participants