Skip to content

Update to Pyunifi2.0#6490

Merged
pvizeli merged 6 commits into
home-assistant:devfrom
finish06:pyunifi2.0
Mar 10, 2017
Merged

Update to Pyunifi2.0#6490
pvizeli merged 6 commits into
home-assistant:devfrom
finish06:pyunifi2.0

Conversation

@finish06
Copy link
Copy Markdown
Contributor

@finish06 finish06 commented Mar 8, 2017

Description:

Update pyUnifi component to use most recent pyUnifi package, i.e. 2.0. The 2.0 pyUnifi package now uses the requests module. Hopefully will help with users have connection issues, however unsure due to inability to reproduce end user errors.

##Documents PR:
home-assistant/home-assistant.io#2222

##Example entry for configuration.yaml (if applicable):

device_tracker:
  - platform: unifi
    host: unifi.ho.me
    username: admin
    password: password
    verify_ssl: False

Checklist:

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

  • New dependencies have been added to the REQUIREMENTS variable ([example][ex-requir]).
  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • New dependencies are only imported inside functions that use them ([example][ex-import]).
  • New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

@homeassistant
Copy link
Copy Markdown
Contributor

Hi @finish06,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@mention-bot
Copy link
Copy Markdown

@finish06, thanks for your PR! By analyzing the history of the files in this pull request, we identified @kk7ds, @fabaff and @balloob to be potential reviewers.

_LOGGER = logging.getLogger(__name__)
CONF_PORT = 'port'
CONF_SITE_ID = 'site_id'
DEFAULT_VERIFY_SSL = False
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Set it to True. We have security opt-out and not opt-in

@finish06
Copy link
Copy Markdown
Contributor Author

finish06 commented Mar 8, 2017

Will also need to update .yml configuration documentation prior to moving forward. Will attempt to update wiki tonight.

@thegame3202
Copy link
Copy Markdown
Contributor

Curious... I wonder if this may fix my issue of devices not reporting "not home" as soon as it disconnects from the AP. They do report "home" as soon as it connects though. Thoughts?

@finish06
Copy link
Copy Markdown
Contributor Author

finish06 commented Mar 9, 2017

@thegame3202 Check out the 'consider_home" parameter: https://home-assistant.io/components/device_tracker/.

Copy link
Copy Markdown
Member

@pvizeli pvizeli left a comment

Choose a reason for hiding this comment

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

Leave a comment

@pvizeli pvizeli merged commit b705b3d into home-assistant:dev Mar 10, 2017
pvizeli pushed a commit that referenced this pull request Mar 10, 2017
* Updated pyUnifi

* Missing comma

* Security opt-out, not opt-in

* Adjust minimal values

* Update to pyUnifi 2.0
@drobtravels
Copy link
Copy Markdown
Contributor

@finish06 I'm curious why you defaulted verify SSL to true?

I'm glad you added this option, but my assumption is the majority of users are running the Unifi controller on their local network with no domain or SSL cert. Since it previously wasn't checked, this ended up a breaking change for many users (See #6589)

My opinion is setting to to false, or perhaps making it a required option, so that the user is warned, would be a saner default.

@finish06
Copy link
Copy Markdown
Contributor Author

@droberts84 My initial decision was to have verify_ssl set to false as default. However, Home-Assistant standard is opt-in security with user choice to opt-out. This approach to security by Home-Assistant is what drove the decision to default true.

@drobtravels
Copy link
Copy Markdown
Contributor

Oh ok, thanks for the explanation!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants