Skip to content
This repository has been archived by the owner on Apr 12, 2022. It is now read-only.

New feature: HTTP proxy config using HomeServerConnectionConfig #519

Merged
merged 5 commits into from
Dec 18, 2019

Conversation

unclejay80
Copy link

With this change it is possible to configure a HTTP proxy (hostname & port) from an app which integrates the SDK

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request updates CHANGES.rst
  • Pull request includes a sign off

Signed-off-by: Jürgen Wischer me@unclejay.de

@bmarty bmarty self-requested a review December 17, 2019 07:15
Copy link
Contributor

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

LGTM.
I've made a few comments.
Regarding the proxy usage, maybe it should also be passed when opening a url connection at those places:

Copy link
Contributor

@bmarty bmarty 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 it's ok to pass the HomeServerConnectionConfig to whoever does network requests.
Please just revert the change on the DataHandler as per the comments.
First I thought about just passing the Proxy, but we may need other parameters. I'm just vigilant, because it also contains the access token, which is quite a sensitive data and I do not want it to be used by mistake. But for the moment it's ok.

Copy link
Contributor

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the update

@bmarty bmarty merged commit 3427bf4 into matrix-org:develop Dec 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants