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

Addons with schema: false are broken in 2021.02.5 #2540

Closed
yozik04 opened this issue Feb 9, 2021 · 30 comments
Closed

Addons with schema: false are broken in 2021.02.5 #2540

yozik04 opened this issue Feb 9, 2021 · 30 comments

Comments

@yozik04
Copy link

yozik04 commented Feb 9, 2021

Describe the issue

Home Assistant addons that does not specify configuration schema seems to be broken. All provided settings are filtered out.

https://developers.home-assistant.io/docs/add-ons/configuration/ documentation tells:

Key Type Required Description
schema dict no Schema for options value of the add-on. It can be false to disable schema validation and use custom options.

If addon uses schema: false it will not be able to read any configuration from options.json
Example config here: https://github.com/ParadoxAlarmInterface/hassio-repository/blob/master/paradox_alarm_interface/config.json

Supervisor just strips all configuration options out.

I think bug was introduced with this change: https://github.com/home-assistant/supervisor/pull/2510/files

Steps to reproduce

  1. Update HA OS from 5.10 to 5.11
  2. Some plugins stop accepting configuration: Stop working after update to HA OS 5.11 ParadoxAlarmInterface/pai#199

Enviroment details

  • Operating System:: RPI4 HA OS
  • Supervisor version:: 2021.02.5
  • Home Assistant version: 2021.2.1

Supervisor logs

Supervisor logs
21-02-08 20:36:01 WARNING (MainThread) [supervisor.addons.options] Unknown options LOGGING_FILE
21-02-08 20:36:01 WARNING (MainThread) [supervisor.addons.options] Unknown options LOGGING_LEVEL_CONSOLE
21-02-08 20:36:01 WARNING (MainThread) [supervisor.addons.options] Unknown options LOGGING_LEVEL_FILE
21-02-08 20:36:01 WARNING (MainThread) [supervisor.addons.options] Unknown options LOGGING_DUMP_PACKETS
21-02-08 20:36:01 WARNING (MainThread) [supervisor.addons.options] Unknown options LOGGING_DUMP_MESSAGES
21-02-08 20:36:01 WARNING (MainThread) [supervisor.addons.options] Unknown options LOGGING_DUMP_STATUS
21-02-08 20:36:01 WARNING (MainThread) [supervisor.addons.options] Unknown options LOGGING_DUMP_EVENTS
21-02-08 20:36:01 WARNING (MainThread) [supervisor.addons.options] Unknown options CONNECTION_TYPE
21-02-08 20:36:01 WARNING (MainThread) [supervisor.addons.options] Unknown options SERIAL_PORT
21-02-08 20:36:01 WARNING (MainThread) [supervisor.addons.options] Unknown options SERIAL_BAUD
21-02-08 20:36:01 WARNING (MainThread) [supervisor.addons.options] Unknown options IP_CONNECTION_HOST
21-02-08 20:36:01 WARNING (MainThread) [supervisor.addons.options] Unknown options IP_CONNECTION_PORT
21-02-08 20:36:01 WARNING (MainThread) [supervisor.addons.options] Unknown options IP_CONNECTION_PASSWORD
21-02-08 20:36:01 WARNING (MainThread) [supervisor.addons.options] Unknown options IP_CONNECTION_SITEID
21-02-08 20:36:01 WARNING (MainThread) [supervisor.addons.options] Unknown options IP_CONNECTION_EMAIL
21-02-08 20:36:01 WARNING (MainThread) [supervisor.addons.options] Unknown options IP_CONNECTION_PANEL_SERIAL
21-02-08 20:36:01 WARNING (MainThread) [supervisor.addons.options] Unknown options IP_CONNECTION_BARE
21-02-08 20:36:01 WARNING (MainThread) [supervisor.addons.options] Unknown options LIMITS
21-02-08 20:36:01 WARNING (MainThread) [supervisor.addons.options] Unknown options SYNC_TIME
21-02-08 20:36:01 WARNING (MainThread) [supervisor.addons.options] Unknown options PASSWORD
21-02-08 20:36:01 WARNING (MainThread) [supervisor.addons.options] Unknown options MQTT_ENABLE
21-02-08 20:36:01 WARNING (MainThread) [supervisor.addons.options] Unknown options MQTT_HOST
21-02-08 20:36:01 WARNING (MainThread) [supervisor.addons.options] Unknown options MQTT_PORT
21-02-08 20:36:01 WARNING (MainThread) [supervisor.addons.options] Unknown options MQTT_USERNAME
21-02-08 20:36:01 WARNING (MainThread) [supervisor.addons.options] Unknown options MQTT_PASSWORD
21-02-08 20:36:01 WARNING (MainThread) [supervisor.addons.options] Unknown options MQTT_RETAIN
21-02-08 20:36:01 WARNING (MainThread) [supervisor.addons.options] Unknown options MQTT_BIND_ADDRESS
21-02-08 20:36:01 WARNING (MainThread) [supervisor.addons.options] Unknown options MQTT_BIND_PORT
21-02-08 20:36:01 WARNING (MainThread) [supervisor.addons.options] Unknown options MQTT_HOMEASSISTANT_AUTODISCOVERY_ENABLE
21-02-08 20:36:01 WARNING (MainThread) [supervisor.addons.options] Unknown options MQTT_PUBLISH_STATUS
21-02-08 20:36:03 WARNING (MainThread) [supervisor.addons.options] Unknown options port

System Information

System Information
arch: aarch64
channel: stable
docker: 19.03.13
features:
- reboot
- shutdown
- services
- network
- hostname
- hassos
hassos: "5.11"
homeassistant: 2021.2.2
hostname: homeassistant
logging: info
machine: raspberrypi4-64
operating_system: Home Assistant OS 5.11
state: running
supervisor: 2021.02.6
supported: true
supported_arch:
- aarch64
- armv7
- armhf
timezone: Europe/Brussels
@yozik04 yozik04 added the bug label Feb 9, 2021
@pvizeli pvizeli removed the bug label Feb 9, 2021
@pvizeli
Copy link
Member

pvizeli commented Feb 9, 2021

That was stale documentation: home-assistant/developers.home-assistant#800

So we fix the bug and not created one :)

@pvizeli pvizeli closed this as completed Feb 9, 2021
@yozik04
Copy link
Author

yozik04 commented Feb 9, 2021

That is evil. I doubt this is a good decision.
I have more than 100 options that people can define and for most people just a 10 is sufficient. Now I will have to show them all 100 and get a lot of reports that something is not working because they have changed a setting without reading documentation.

@frenck
Copy link
Member

frenck commented Feb 9, 2021

You don't have to show them all? You can still make optional settings in a schema (I do that all the time).

@yozik04
Copy link
Author

yozik04 commented Feb 9, 2021

Any logical reason why extra options are bad?

@frenck
Copy link
Member

frenck commented Feb 9, 2021

Extra options are not bad? You can mark them optional.

@mKeRix
Copy link

mKeRix commented Feb 9, 2021

I would lobby for bringing the old behavior back as well - this change has broken the room-assistant add-on for a lot of users without warning (or at least I didn't read anything about this before my users approached me on GitHub with the issue). Bringing the complex option tree under schema validation is quite difficult. Last time I tried I failed to depict the YAML tree using the schema correctly, which is the whole reason I disabled it.

I would be interested to hear more about the reasoning for this change.

EDIT:

Only nested arrays and dictionaries are supported with a deep of two size

This alone makes it impossible for me to write a schema for the room-assistant config. Only way to make it work is by making the application config YAML an inline document, which is a breaking change and isn't great from a UX point of view imo.

EDIT 2:

According to a conversation on Discord this limitation doesn’t really exist and was more meant as a recommendation. That would make some parts of my previous arguments void, but it still doesn’t allow for some flexibility features such as polytypes. Writing a schema is also still a time consuming task for projects with bigger configs.

@yozik04
Copy link
Author

yozik04 commented Feb 9, 2021

I really doubt if I will be able to create a valid json schema that would cover all the options I have in my addon. JSON schema format is so limited.

@frenck
Copy link
Member

frenck commented Feb 9, 2021

JSON schema format is so limited

If there is schema validation missing, please raise that separately. Thanks 👍

@gabrielmalherbe
Copy link

Forced schema validation has broken PAI add-on. I cannot seem to toll back to a previous version of Supervisor either using the HA CLI command. What can I do to roll back or should I apply an old snapshot?

@clau-bucur
Copy link

@pvizeli Even if you see your approach as the correct behavior, what was done was a breaking change without a prior notification.
IMHO, you should mark "schema: false" as deprecated, specify some kind of deadline for removal and allow devs of the addons that use it to align, then when your deadline arrived you can remove it.
It's really a bad approach to introduce such drastic changes in bevahior without prior notification.

I can say I'm rather new to HA (using it for about two years) but the number of times the Supervisor updates is breaking things is pretty uneasy.
And on-top of that you can't downgrade the Supervisor so you get back to a release which was working for you (until the broken thing gets fixed) because there's the auto-upgrade feature (which I understand the reason for, sure).

Maybe a "postpone auto-updates for x days" CLI option when downgrading the Supervisor would be helpful. Even Windows 10 with their aggressive Updates allows for posponing them :). Think about it.

@yozik04
Copy link
Author

yozik04 commented Feb 10, 2021

@frenck and @pvizeli So with this "fix" you have broken 3 addons. Can you please at least tell us why this "fix" was required and what it actually solves?
You just forced addon developers to write and maintain two separate config validation solutions in a different repos. Keeping them in sync is extra useless effort.
Why supervisor deals with validation at all. It is not its responsibility. Only addon knows what setting combination is valid and what is not. Ok, let it do it for addons with simple configuration if a dev wants it. But why it must enforce schema validation for all? I do not understand.

@frenck How can I specify in schema so a sub dict could have any keys and values?

{
"a": {<anykey=anyvalue_or_sub_dict>,...}
}

And please reopen this issue if you do not want people to create another dozen of dupes. Our debate is in progress. Closing the issue before a solution is found is a bad manner.

@frenck
Copy link
Member

frenck commented Feb 10, 2021

@yozik04 A schema can have dicts and subvalues, that has been available since the early beginnings.
For example https://github.com/hassio-addons/addon-ftp/blob/main/ftp/config.json#L34

But a schema is explicit, "any" is not part of that.

@frenck
Copy link
Member

frenck commented Feb 10, 2021

Why supervisor deals with validation at all. It is not its responsibility.

Yes, it is, it always has been.

@yozik04
Copy link
Author

yozik04 commented Feb 10, 2021

@frenck And can we have that "any" part? Can you or @pvizeli implement it? Some "json" or what ever type?

@frenck
Copy link
Member

frenck commented Feb 10, 2021

IMHO, that is a bad idea in general. I (personally) would not support it.

@yozik04
Copy link
Author

yozik04 commented Feb 10, 2021

@frenck Is it possible to make a configuration sub tree optional?

{
"a": {"b":"c"},
"optional": {"a": "b"}
}

@frenck
Copy link
Member

frenck commented Feb 10, 2021

That is the same question you have asked above twice already in different words. If someone decides it should be part of the add-on configuration 🤷‍♂️ As for my personal opinion, I would not support that.

@yozik04
Copy link
Author

yozik04 commented Feb 10, 2021

No I mean this way:

{
    "a": {
        "b": ["int"],
        "optional": ["int"]
    },
}

Some hardware that we support does not have "optional" sub configuration.

@frenck
Copy link
Member

frenck commented Feb 10, 2021

Options can be marked with a ? to make them optional.

@yozik04
Copy link
Author

yozik04 commented Feb 10, 2021

So only leaf nodes can be optional?

@frenck
Copy link
Member

frenck commented Feb 10, 2021

The optionality is not limited to roots or leaves. It applies to a node by itself.

@yozik04
Copy link
Author

yozik04 commented Feb 10, 2021

{
    "a": {
        "b": ["int"],
        "optional?": ["int"]
    },
}

So optionality in keys also supported? See optional?.

@frenck
Copy link
Member

frenck commented Feb 10, 2021

{
    "a": {
        "b": ["int"],
        "optional": ["int?"]
    },
}

that optional, is already optional?

As one doesn't have to provide values?

optional: []

@yozik04
Copy link
Author

yozik04 commented Feb 10, 2021

@frenck "email?" and "device?" types does not want to be optional. Validation fails if we do not provide a value.

@frenck
Copy link
Member

frenck commented Feb 10, 2021

all provided validation types should be able to support the optional flag. If you conclude that is not the case, please raise a separate issue for that with a reproduction scenario, so we can take a look at the reason for that.

@clau-bucur
Copy link

clau-bucur commented Feb 10, 2021

@frenck How does one specify an optional string value with default value "null" ?
This one, for example, does not work, it makes LOGGING_FILE mandatory:

"options": {
    "LOGGING_FILE": null
},
"schema": {
    "LOGGING_FILE": "str?"
}

@frenck
Copy link
Member

frenck commented Feb 10, 2021

@clau-bucur Lets not stack issues. Please raise a separate issue if you have a specific case. Thanks 👍

@clau-bucur
Copy link

I'd say was a simple question related to the issue at hand. But ok. Thanks.

@yozik04
Copy link
Author

yozik04 commented Feb 10, 2021

@frenck Thank you for your answers. It was helpful. But you really like to skip questions or answer partially... That makes a negative effect.

@frenck
Copy link
Member

frenck commented Feb 10, 2021

I'm sorry, it helps if an issue is created for a single thing. If you need a chat or support, please join our Discord server, as we have developer channels on there.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants