Skip to content

Strip scope_id from IPv6 address if given.#1020

Merged
bdraco merged 4 commits into
python-zeroconf:masterfrom
StevenLooman:master
Oct 30, 2021
Merged

Strip scope_id from IPv6 address if given.#1020
bdraco merged 4 commits into
python-zeroconf:masterfrom
StevenLooman:master

Conversation

@StevenLooman
Copy link
Copy Markdown

This prevents a crash on Python versions < 3.9 when scope_id is supplied in the address, and also fixes finding the related interface.

In preparation of home assistant moving to minimal-Python 3.9 requirement, and enabling scope_ids via https://github.com/home-assistant/core/blob/a90c8ab5584ce50e8dfae67790cb56f3d1d1ea86/homeassistant/components/network/__init__.py#L55.

This prevents a crash on Python versions < 3.9.
@StevenLooman
Copy link
Copy Markdown
Author

I think @bdraco should be looking at this. It is as a preparation fixing home-assistant/core#56723, and better supporting IPv6 in https://github.com/StevenLooman/async_upnp_client.

@StevenLooman
Copy link
Copy Markdown
Author

StevenLooman commented Oct 30, 2021

Related PR from async-upnp-client: StevenLooman/async_upnp_client#108, though still work in progress.

Comment thread zeroconf/_utils/net.py Outdated


def ip6_to_address_and_index(adapters: List[Any], ip: str) -> Tuple[Tuple[str, int, int], int]:
ip = ip[:ip.index('%')] # Strip scope_id.
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.

Is it possible it will be passed in without the '%' and we should only strip it if present?

Copy link
Copy Markdown
Author

@StevenLooman StevenLooman Oct 30, 2021

Choose a reason for hiding this comment

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

Oh certainly, not sure why I thought this was a good idea. Thank you for noticing.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Oct 30, 2021

Codecov Report

Merging #1020 (919e5da) into master (0fdcd51) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1020   +/-   ##
=======================================
  Coverage   99.68%   99.68%           
=======================================
  Files          23       23           
  Lines        2524     2526    +2     
  Branches      412      413    +1     
=======================================
+ Hits         2516     2518    +2     
  Misses          5        5           
  Partials        3        3           
Impacted Files Coverage Δ
zeroconf/_utils/net.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0fdcd51...919e5da. Read the comment docs.

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Oct 30, 2021

Should be good to go once coverage is added.

We have 100% coverage here sans python 3.6 (which is being dropped in #1009)

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Oct 30, 2021

I'll be AFK for a bit

Feel free to ping me on discord if you need the CI started.

@StevenLooman
Copy link
Copy Markdown
Author

I think its ok now. No rush, later is ok for me. Doubt this'll land in next hass release anyway?

Like the CI, on my TODO list: add to async-upnp-client. :)

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Oct 30, 2021

I think its ok now. No rush, later is ok for me. Doubt this'll land in next hass release anyway?

Looks good now. I'll do a release now and bump it in HASS so it doesn't block you later.

@StevenLooman
Copy link
Copy Markdown
Author

Looks good now. I'll do a release now and bump it in HASS so it doesn't block you later.

Great, thanks! Though I'll create the PR for the mentioned part in home assistant for release 2022.1 as Python3.8 support is dropped only then. Again, no rush.

@bdraco bdraco merged commit 686febd into python-zeroconf:master Oct 30, 2021
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