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

Adjust REST schema validation #81723

Merged
merged 2 commits into from
Nov 8, 2022
Merged

Conversation

epenet
Copy link
Contributor

@epenet epenet commented Nov 7, 2022

Proposed change

Linked to #81551

In 2022.10, passing an empty configuration to rest resulted in a successful setup, with a failing coordinator and no sensors.

rest:

In 2022.11, code was adjusted on the coordinator to ensure we had a valid resource before creating the dataclass, which resulted in a HomeAssistantError:

raise HomeAssistantError("Resource not set for RestData")

The side effect of this change is that it highlited invalid configurations in the real world (previously throwing errors in logs, but probably ignored, now failing).

This PR adjusts the schema validation to ensure an empty dict first gets converted to an empty list rather than get converted to a list with a single dict

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)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

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.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

@bdraco
Copy link
Member

bdraco commented Nov 7, 2022

A hard fail is better here since it can be caught by a configuration check and the error doesn't get hidden away leaving the user to guess what went wrong. It just isn't so much fun on an upgrade.

I'd go back an relabel the existing case a breaking change and update the release notes so it makes the one time pain a bit easier to resolve.

@epenet epenet marked this pull request as draft November 7, 2022 14:45
@elupus
Copy link
Contributor

elupus commented Nov 7, 2022

Why would an empty test block be a fail? It should just be a loaded component with no configured sensors.

@epenet
Copy link
Contributor Author

epenet commented Nov 7, 2022

Why would an empty test block be a fail? It should just be a loaded component with no configured sensors.

If the schema validation returned an empty list that would work. But I'm not sure how to adjust it accordingly.

@michaelblight
Copy link

I think it's a bug that it can't cope with an empty included file. I also have

group: !include groups.yaml

And my "groups.yaml" file is empty, but there is no error. So I think it needs to be able to cope with empty file without an error or warning.

@epenet epenet marked this pull request as ready for review November 8, 2022 06:45
@epenet epenet added this to the 2022.11.2 milestone Nov 8, 2022
@epenet
Copy link
Contributor Author

epenet commented Nov 8, 2022

I think the latest commit 7bd7b2f resolve this issue.

If an empty dict is passed to the schema validation, it now gets converted to an empty list.

Without this patch, it was being converted to a list with a single dict with injected defaults:
[{'verify_ssl': True, 'timeout': 10, 'method': 'GET'}],

@balloob balloob merged commit c585817 into home-assistant:dev Nov 8, 2022
@epenet epenet deleted the fix-rest-init branch November 8, 2022 10:19
frenck pushed a commit that referenced this pull request Nov 8, 2022
@frenck frenck mentioned this pull request Nov 8, 2022
@epenet epenet mentioned this pull request Nov 8, 2022
19 tasks
@github-actions github-actions bot locked and limited conversation to collaborators Nov 9, 2022
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.

Sensor.Rest not able to start after 2022.11.1
6 participants