-
Notifications
You must be signed in to change notification settings - Fork 419
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
refactor repeated SSL/auth cruft for HASS connection #1583
Conversation
Looks like I need to black format, will do that and update in a couple hours. |
else: | ||
cert_path = False # noqa: F841 | ||
# if we get a request for not our namespace something has gone very wrong | ||
assert namespace == self.namespace |
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.
I left these as an assert because iiuc we should really never get a mismatch or that indicates a serious logic error. If there's a more appropriate exception to throw I'm happy to use another.
self.cert_verify = args.get("cert_verify") | ||
self.commtype = args.get("commtype", "WS") | ||
self.ha_key = args.get("ha_key") | ||
self.ha_url = args.get("ha_url", "") |
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.
This is a behavior change. Previously, if you didn't specify the ha_url, it would crash on startup with "no attribute 'ha_url'". Now it will fail to connect instead.
Ok tested the PR @zanfur and from initial tests looks ok. But I and @acockburn need to be sure of some stuffs before it can be merged as @acockburn need to give the final sign-off anyway. The first thing will be to avoid the exposure of the ws object as its not a good idea to allow for users to be able to create new ones at will |
This PR doesn't change anything about how exposed the ws object is, just refactors how it's created. It's still just a member variable on the plugin object. I considered not even saving it there, because it isn't actually necessary, but that's how it was previously so I left it. |
I don't see any immediate problems here, if @Odianosen25 says it tests out OK I am happy to merge it into dev for further testing |
This pulls all the copypasta auth and ssl handling into a single place for the aiohttp session, and as a bonus implements the todo around using the cert_path argument. Additionally, put the ha_url into the session's base_url rather than prepending it everywhere.
With this change, now any appdaemon app can use
plugin.session.request("get", "/api/whatever")
to make arbitrary HA rest calls through the AD session, allowing full access to HA data even if there's no integrated AD support for it (like Devices or Areas or search).