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

feat: expose flag to disable strict name checking in service registration #1215

Merged
merged 3 commits into from Aug 13, 2023

Conversation

azogue
Copy link
Contributor

@azogue azogue commented Aug 4, 2023

closes #1046

@garethsb
Copy link

garethsb commented Aug 7, 2023

👍 Thanks for this, hope it can be merged soon. For working with existing protocols that use strictly-invalid service types, we have had to adjust our monkeypatch several times. 🐒😄

@garethsb
Copy link

@bdraco thanks for considering this PR when you have time

@bdraco
Copy link
Member

bdraco commented Aug 10, 2023

This looks fine. It needs explicit test coverage for a few more places before merging.

@bdraco bdraco changed the title Expose flag to disable strict name checking in service registration feat: expose flag to disable strict name checking in service registration Aug 10, 2023
@codecov
Copy link

codecov bot commented Aug 10, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (0094e26) 99.78% compared to head (c8996e6) 99.78%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1215   +/-   ##
=======================================
  Coverage   99.78%   99.78%           
=======================================
  Files          22       22           
  Lines        2757     2757           
  Branches      480      480           
=======================================
  Hits         2751     2751           
  Misses          3        3           
  Partials        3        3           
Files Changed Coverage Δ
src/zeroconf/asyncio.py 100.00% <ø> (ø)
src/zeroconf/__init__.py 100.00% <100.00%> (ø)
src/zeroconf/_core.py 99.77% <100.00%> (ø)
src/zeroconf/_services/info.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bdraco
Copy link
Member

bdraco commented Aug 10, 2023

The following can be called directly so they should get a simple test to make sure disabling works when called directly:

  • bound async_check_service

These are exposed at the top level for backwards compat which we likely need to maintain forever so they should get a test as well:

@azogue azogue force-pushed the chore/expose-strict-name-flag branch from 0d7771e to c8996e6 Compare August 12, 2023 11:18
@azogue
Copy link
Contributor Author

azogue commented Aug 12, 2023

The following can be called directly so they should get a simple test to make sure disabling works when called directly:

  • bound async_check_service

These are exposed at the top level for backwards compat which we likely need to maintain forever so they should get a test as well:

Hi @bdraco, please take a look now 👍

I was not sure where to put those calls:

  • Added explicit call to Zeroconf.async_check_service in test_asyncio.py
  • Added a new unit test in utils/test_name.py to check explicit calls to service_type_name and instance_name_from_service_info

Also, I re-pushed the branch to rename the old 2 commits (just de-capitalized the titles, after seeing a CI fail with the GAs 😅)

@bdraco
Copy link
Member

bdraco commented Aug 13, 2023

Tested on production. Didn't find any issues

Thanks @azogue

@bdraco bdraco merged commit 5df8a57 into python-zeroconf:master Aug 13, 2023
34 checks passed
@azogue azogue deleted the chore/expose-strict-name-flag branch August 14, 2023 06:09
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.

Add method to disable strict service name checking.
3 participants