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

Added optional setting http_proxy to push_image #144

Merged
merged 10 commits into from
Sep 1, 2024

Conversation

xNUTx
Copy link
Contributor

@xNUTx xNUTx commented Aug 15, 2024

Original discussion here: #117

I had it in 'main' instead of a separate branch, so it got a little confusing making patches for other things...

Functional it has no changes compared to the original PR.

This will allow the user to set up an external reverse proxy to have the HA instance operate on HTTPS and push_image use that reverse proxy to avoid using HTTPS traffic processing on the plate.

Besides now working around a bug in the libraries that kills SSL traffic on the plates, it will also allow the plate not to deal with SSL encrypted connections at all (even when it is working again) to save processing power.

HA Automation Action example:

data:
  obj: p12b1
  image: >-
    https://myhainstance:portnumber{{
    state_attr('image.doorbell_event_image','entity_picture') }}
  http_proxy: http://internaladdress:portnumber
  width: 480
  height: 320
  fitscreen: 1
target:
  entity_id: openhasp.displayname
enabled: true
action: openhasp.push_image

custom_components/openhasp/__init__.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@dgomes dgomes left a comment

Choose a reason for hiding this comment

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

Still don't see the point as discussed in #117 but technically is OK

@xNUTx
Copy link
Contributor Author

xNUTx commented Aug 20, 2024

Still don't see the point as discussed in #117 but technically is OK

If you are really that heartfelt about the naming, feel free to modify it as you see fit. Functionality is what matters, naming is trivial imho.

@fvanroie
Copy link
Collaborator

So,can this be merged or not?

@xNUTx
Copy link
Contributor Author

xNUTx commented Aug 30, 2024

So,can this be merged or not?

From my point of view, yes.

Technically it can be according to @dgomes.
If he insists on changing the name of the config variables he can choose to do so, I don't care about the naming that much and I personally need this feature in the HA component for it to work properly in my case and I am getting tired of making modifications every time there is an update on HA or it's components overwriting my required changes.

@fvanroie
Copy link
Collaborator

I'm not sure I understand the issue myself. I'm preparing for the HA 2024.9.0 release, so it would be a good time to merge.

@dgomes
Copy link
Collaborator

dgomes commented Aug 30, 2024

just rename it to what it is: expose_on_address

@fvanroie
Copy link
Collaborator

fvanroie commented Aug 30, 2024

Okay, so I think I understand it better now. While the functionality is there, the naming of this line is not ideal:

ATTR_PROXY = "http_proxy"

As the link is not a real proxy server, but it is just being hosted on a different link...

Can I offer my 2cents: what about server_url or base_url? That seems to be a more universal naming of what the feature does.

In any case the feature need to be documented with an example.

@xNUTx
Copy link
Contributor Author

xNUTx commented Aug 30, 2024 via email

@fvanroie fvanroie merged commit f8386d3 into HASwitchPlate:main Sep 1, 2024
2 checks passed
@fvanroie
Copy link
Collaborator

fvanroie commented Sep 1, 2024

It seems we are running in circles here...

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

Successfully merging this pull request may close these issues.

3 participants