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

Pass throught custom http headers as transport configuration #1630

Closed
wants to merge 4 commits into from

Conversation

fungs
Copy link
Contributor

@fungs fungs commented Mar 7, 2024

This minimalistic PR adds the possibility to specify custom http headers as transport configuration.

Example:

component = AutobahnComponent(
  transports={
    "type": "websocket",
    "url": "YOUR URL",
    "headers": {"Authorization": "YOUR CUSTOM AUTHORIZATION"},
  },
    ...
)

The patch does not change the internal logic, the configuration is automatically passed and used in the websocket connection factory. I tried to keep the changes minimal (although I was tempted to apply the boy scout rule and do some tidy up work).

Copy link
Contributor

@meejah meejah left a comment

Choose a reason for hiding this comment

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

Looks good to me overall; inline comment about an additional check.

@@ -160,6 +160,8 @@ def _create_transport(index, transport, check_native_endpoint=None):
raise ValueError(
'options must be a dict, not {}'.format(type(options))
)

headers = transport.get("headers", None)
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably want a check in "rawsocket" (i.e. headers should be None if kind == 'rawsocket' because headers doesn't make sense for that case).

@oberstet oberstet self-requested a review March 8, 2024 02:00
Copy link
Contributor

@oberstet oberstet left a comment

Choose a reason for hiding this comment

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

thanks a lot! I like the "minimalistic" approach. +1 for the additional check that @meejah mentioned as this is a transport-specific feature while "component" is exactly about abstracting away "all" transport details (like in general with WAMP at the app level, above transports). for me, a simple/deadly "assert" would be fine .. that's enough to make it fail and the user/developer aware.

@oberstet
Copy link
Contributor

oberstet commented Mar 8, 2024

rgd

https://github.com/crossbario/autobahn-python/actions/runs/8187466713/job/22419880678?pr=1630#step:8:116

pls see

#1628 (comment)

TLDR: this is fallout from a breaking change in pytest-asyncio ...

@fungs
Copy link
Contributor Author

fungs commented Mar 9, 2024

I will do the polishing next week.

@fungs
Copy link
Contributor Author

fungs commented Mar 20, 2024

I will do the polishing next week.

Here we go, I raised a ValueError, as for the other incompatible arguments and removed the default None return value from get().

@oberstet
Copy link
Contributor

flake is complaining about WS https://github.com/crossbario/autobahn-python/actions/runs/8357659564/job/22881485753?pr=1630#step:6:71

the asyncio issue is related to the latest (breaking) changes in pytest-asyncio rgd event loop setup I guess ..

@@ -229,6 +229,8 @@ def _create_transport(index, transport, check_native_endpoint=None):
endpoint_config = transport['endpoint']
if 'serializers' in transport:
raise ValueError("'serializers' is only for websocket; use 'serializer'")
if headers is None:
Copy link
Contributor

Choose a reason for hiding this comment

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

is not None I think..?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh man...
thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should be fixed

@oberstet
Copy link
Contributor

  autobahn/wamp/component.py:124:9: E123 closing bracket does not match indentation of opening bracket's line
  autobahn/wamp/component.py:163:1: W293 blank line contains whitespace
  1     E123 closing bracket does not match indentation of opening bracket's line
  1     W293 blank line contains whitespace
  flake8: exit 1 (4.27 seconds) /home/runner/work/autobahn-python/autobahn-python> flake8 -v --statistics --ignore=E402,E501,E722,E741,N801,N802,N803,N805,N806,N815,N818 --exclude 'autobahn/wamp/message_fbs.py,autobahn/wamp/gen/*' autobahn pid=2865

@oberstet
Copy link
Contributor

rebased to master and rerunning CI .. let's see

#1638

@oberstet
Copy link
Contributor

merged via #1638 - thanks for contributing!

@oberstet oberstet closed this Apr 13, 2024
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.

3 participants