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.

Sign up for GitHub

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

Jump to bottom

Disable creating port mappings from UI, add discovery from component #18565

Merged
merged 4 commits into from Dec 21, 2018
Merged

Disable creating port mappings from UI, add discovery from component #18565

merged 4 commits into from Dec 21, 2018

Conversation

StevenLooman
Copy link
Contributor

@StevenLooman StevenLooman commented Nov 18, 2018 •
edited by MartinHjelmare

Description:

Disable creating port mappings from UI (see #17937)
Add discovery from component (see #17400)
Upgrade to async_upnp_client==0.13.3 to support async discovery
Possible fix for #18066: SSL being required when talking to non-SSL-enabled services

Related issue (if applicable):
fixes #17937, fixes #17400

Pull request in home-assistant.io with documentation (if applicable): -

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

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

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

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

@ghost ghost added the in progress label Nov 18, 2018
@StevenLooman
Copy link
Contributor Author

@dgomes

@StevenLooman
Copy link
Contributor Author

Please do not merge. I'm not happy with the state of the flow.

@frenck
Copy link
Member

frenck commented Nov 19, 2018

Please do not merge. I'm not happy with the state of the flow.

Please add WIP: to the title in that case...

@MartinHjelmare MartinHjelmare changed the title Disable creating port mappings from UI, add discovery from component WIP: Disable creating port mappings from UI, add discovery from component Nov 20, 2018
@StevenLooman StevenLooman requested a review from a team as a code owner November 24, 2018 16:22
@StevenLooman
Copy link
Contributor Author

@dgomes Please review. Any comment is welcome.

The component has been rewritten to use the DiscoveryFlowHandler, simplifying things for the component itself. Also, now the component creates a device in the device registry, which the sensors are using/reporting.

@StevenLooman StevenLooman changed the title WIP: Disable creating port mappings from UI, add discovery from component Disable creating port mappings from UI, add discovery from component Nov 24, 2018
dgomes
dgomes approved these changes Nov 24, 2018
View reviewed changes
Copy link
Contributor

@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.

LGTM!

homeassistant/components/upnp/__init__.py Outdated Show resolved Hide resolved
@balloob
Copy link
Member

balloob commented Nov 27, 2018

I think that we need to login to Lokalise to remove the no longer needed translation keys. Just removing the files won't help as they are not the source of truth.

@StevenLooman
Copy link
Contributor Author

I think that we need to login to Lokalise to remove the no longer needed translation keys. Just removing the files won't help as they are not the source of truth.

I don't get what you're trying to say.

I removed the files as these were:

  1. incomplete for most languages, and
  2. no longer up to date, and
  3. Not added by me. I.e., new (and better, given these were obsolete/not up to date) translations are added by others.

@balloob
Copy link
Member

balloob commented Nov 28, 2018

Changing the translation files has no effect at all. We just run script/translations_download and they will be back, because Lokalise is the source of truth.

@balloob
Copy link
Member

balloob commented Nov 28, 2018

And because the are no longer up to date because you changed the keys, you have to log in to Lokalise and update them there

@StevenLooman
Copy link
Contributor Author

Ok, thank you for explaining. I've joined the backend project. It seems that I cannot change any keys in Localise.
I will add (new) translations for the component when possible.

@ghost ghost removed the in progress label Nov 28, 2018
@StevenLooman
Copy link
Contributor Author

Wrong button...

@StevenLooman StevenLooman reopened this Nov 28, 2018
@ghost ghost added the in progress label Nov 28, 2018
@dgomes
Copy link
Contributor

dgomes commented Dec 5, 2018

@StevenLooman you need to remove the translations from the PR in order for this PR to be merged

@StevenLooman
Copy link
Contributor Author

Done!

@StevenLooman
Copy link
Contributor Author

Minor fix to async_upnp_client.

@dgomes
Copy link
Contributor

dgomes commented Dec 7, 2018

@StevenLooman make sure you remove the commits that remove the translation files

@StevenLooman
Copy link
Contributor Author

StevenLooman commented Dec 9, 2018 •
edited

Can we restart travis? @dgomes
It seems something unrelated has failed.

@MartinHjelmare
Copy link
Member

Restarted the job.

@balloob
Copy link
Member

balloob commented Dec 10, 2018

This PR now removes translations instead of not changing them in this PR.

@StevenLooman
Copy link
Contributor Author

StevenLooman commented Dec 10, 2018 •
edited

I must have mis-read @dgomes' comment. I can only remove the commit which deleted the translations, not which altered them as it contains more changes.

Is this mergable now?

@StevenLooman
Copy link
Contributor Author

I've re-applied all the changes. No translations are touched.

@StevenLooman
Copy link
Contributor Author

Anything I can do?

balloob
balloob reviewed Dec 14, 2018
View reviewed changes
homeassistant/components/sensor/upnp.py
(DOMAIN_UPNP, self.unique_id)
},
'name': self.name,
'via_hub': (DOMAIN_UPNP, self._device.udn),
Copy link
Member

Choose a reason for hiding this comment

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

I see that you register a via_hub here but that device is not created anywhere ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is created in the upnp component itself

@dgomes dgomes merged commit 501b3f9 into home-assistant:dev Dec 21, 2018
@ghost ghost removed the in progress label Dec 21, 2018
@StevenLooman
Copy link
Contributor Author

Thanks!

dshokouhi pushed a commit to dshokouhi/home-assistant that referenced this pull request Dec 25, 2018
@StevenLooman @dshokouhi
Disable creating port mappings from UI, add discovery from component (h…
ecc9a5a 
…ome-assistant#18565)

* Disable creating port mappings from UI, add discovery from component

* Remove unused constant

* Upgrade to async_upnp_client==0.13.6 and use manufacturer from device

* Upgrade to async_upnp_client==0.13.7
@StevenLooman StevenLooman deleted the igd_discovery branch January 8, 2019 19:20
@balloob balloob mentioned this pull request Jan 10, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@balloob balloob balloob left review comments

@MartinHjelmare MartinHjelmare MartinHjelmare left review comments

@dgomes dgomes dgomes approved these changes

@ammarfaizi2 ammarfaizi2 ammarfaizi2 approved these changes

Assignees
No one assigned
Labels
cla-signed integration: discovery integration: dlna_dmr integration: upnp
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@StevenLooman @frenck @balloob @dgomes @MartinHjelmare @ammarfaizi2 @homeassistant