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

Use latest version of python-pushover (forked) to fix issue with diff… #31647

Merged
merged 4 commits into from Feb 10, 2020

Conversation

@SoftXperience
Copy link
Contributor

SoftXperience commented Feb 8, 2020

…erent API tokens. (https://community.home-assistant.io/t/different-applications-in-pushover/6985)

Proposed change

There is a problem in the pushover integration. The underlying python lib stored the API token globally, so all notifies used one same API token, instead of possibly different tokens configured.
The code owner of the underlying lib already fixed it in the code, but did not provide a new release eiter in github nor in pypi, so I couldn't use that.
That's why I forked that repo and published the latest version on pypi.
There were also some small changes in the integration needed to reflect the new version.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #11641
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum
@project-bot project-bot bot added this to Needs review in Dev Feb 8, 2020
@balloob

This comment has been minimized.

Copy link
Member

balloob commented Feb 8, 2020

We won't change libraries to forks. They tend to die and we're stuck with them.

Please list all the efforts you have done to reach out to the author to request a new version of PyPI.

@SoftXperience

This comment has been minimized.

Copy link
Contributor Author

SoftXperience commented Feb 8, 2020

I created a new issue on the project and didn't get a response. The last commit is over 2 years ago, the readme is not reflecting the latest code changes. Also if you look at the ha issue, there was already another user that tried to fix it and was always waiting to get his PR accepted. So it looks very dead to me...

But if you don't like what I did, what should I do instead. Write and publish a lib on my own (basically the same like the fork I did now)? Or look for another pushover library? Which may lead to a lot more code changes in the integration...

@balloob

This comment has been minimized.

Copy link
Member

balloob commented Feb 9, 2020

For us to accept a PR to change the lib of an integration:

  • link to proof that the project has been unmaintained
  • list efforts taken to reach out to maintainer to get added as maintainer
  • if you fork it, give it a new name. We don't want one off forks in our code base, they always go unmaintained.
@SoftXperience

This comment has been minimized.

Copy link
Contributor Author

SoftXperience commented Feb 10, 2020

Hmm ok, I'll try:
There is a community thread from 2016, still unresolved: https://community.home-assistant.io/t/different-applications-in-pushover/6985
There is an issue from Jan 2018, where a user tried to fix it by providing a PR to the underlying library: #11641
The PR was closed by the user after no reaction and no merge after 1 year: Thibauth/python-pushover#27

I opened another issue on that project to try to reach out to the maintainer, but no reaction at all: Thibauth/python-pushover#32

So I also consider this project as abandoned.

But if you don't like forks, here is another try: I'm now using the obviously still maintained library pushover_complete and rewrote the integration to use it.

Maybe you can now check the PR...

@springstan springstan mentioned this pull request Feb 10, 2020
8 of 20 tasks complete
# If attachment is a URL, use requests to open it as a stream.
if data[ATTR_ATTACHMENT].startswith("http"):
if image.startswith("http"):
try:
response = requests.get(
data[ATTR_ATTACHMENT], stream=True, timeout=5

This comment has been minimized.

Copy link
@balloob

balloob Feb 10, 2020

Member

Remove this. We should not allow Home Assistant to download URLs and then send it out in a notification. Sending out urls is fine. Sending out from a whitelisted path is fine too.

This comment has been minimized.

Copy link
@SoftXperience

SoftXperience Feb 10, 2020

Author Contributor

Hmm, I'm fine with removing it, I'm not using it anyways.

But this was also possible before.

And I think it is not possible to call from outside or without authentication, so I guess it's not harmful? What about sending a picture from a camera with the notification? I guess it would be useful to download urls then?

This comment has been minimized.

Copy link
@balloob

balloob Feb 10, 2020

Member

It was possible before but it will be removed from all notification integrations. It's dangerous because the Home Assistant machine can now download anything, including pages that are localhost only and send it to another computer.

This comment has been minimized.

Copy link
@SoftXperience

SoftXperience Feb 10, 2020

Author Contributor

Agreed. Now it only checks agains whitelisted path and refuse other attachments.

@balloob

This comment has been minimized.

Copy link
Member

balloob commented Feb 10, 2020

This is a way better approach. One final comment, ok to merge afterwards.

Marcus Terasa added 2 commits Feb 10, 2020
Marcus Terasa
Dev automation moved this from Needs review to Reviewer approved Feb 10, 2020
@balloob balloob merged commit d55846c into home-assistant:dev Feb 10, 2020
9 checks passed
9 checks passed
CI Build #20200210.66 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
codecov/patch Coverage not affected when comparing 0dd151c...e20f3ec
Details
codecov/project 94.65% (target 90%)
Details
Dev automation moved this from Reviewer approved to Done Feb 10, 2020
@lock lock bot locked and limited conversation to collaborators Feb 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.