Support reloading allow all imports through reload service and config entry options UI

[raman325]

As discussed in #39 , this change makes it possible to reload allow_all_imports via the reload service and also to configure it using the options menu for users who set up the entry through the UI (options will not work for users who do import, because when a user runs the reload service, and there is a mismatch between what's in the config and what was set in options, it would be impossible to know which one takes precedence. The config flow handles this gracefully and tells the user this if they attempt to access options for a config entry they defined in configuration.yaml).

I'm marking this as a draft because I discovered that my implementation won't work without home-assistant/core#41875

I can change the implementation to avoid this, but it would be a less desirable user experience so I am hoping that PR gets accepted.

[craigbarratt]

Thanks for the PR. Is the test failure related to home-assistant/core#41875?

Even if home-assistant/core#41875 gets accepted, what will happen with older versions of HASS?

[raman325]

Yes it is, and that's a fair question - for the two cases where I abort the flow (when someone tries to update through the UI an entry that was created through configuration.yaml or when someone submits the change but no change has actually been applied (e.g. "Allow all imports?" stays checked)), the UI would show an endless spinner and the backend would throw an exception.

There are two options here:

1. Update hacs.json to make the minimum version of HA the one that this PR gets included for
2. Use the slightly less friendly user experience version which is backwards compatible. Basically, instead of saying that the update was aborted, it would act like the update was successful, but I would still display a message to the user explaining that no update will occur.

Which would you prefer?

[craigbarratt]

I'd definitely prefer the backward compatible approach.

[craigbarratt]

Thanks!

So I can update the docs, I wanted to make sure I fully understand how yaml config and config flow work together or are not compatible.

• if a user uses config flow initially, is it possible to later add yaml pyscript apps parameters? If so, can allow_all_imports only be changed via config flow (since it was first set there), or can it also later be set in yaml?
• if a user uses yaml initially, is it true that you can't use config flow later?
• does reinstall allow the user to switch from one method of config to the other?

[raman325]

Thanks!

So I can update the docs, I wanted to make sure I fully understand how yaml config and config flow work together or are not compatible.

• if a user uses config flow initially, is it possible to later add yaml pyscript apps parameters? If so, can allow_all_imports only be changed via config flow (since it was first set there), or can it also later be set in yaml?

Yes, regardless of configuration type, it is possible to add yaml pyscript apps parameters. If the user sets up the configuration through the UI (via config flow), then they can only manage allow_all_imports via config flow. Hopefully the reasoning I provided in the initial PR description makes sense, but if not, I can attempt to re-explain.

• if a user uses yaml initially, is it true that you can't use config flow later?

Correct - at least for now. Same reason as above - if you are using yaml, it's difficult to know what should be considered the source of truth, the yaml or the config entry.

• does reinstall allow the user to switch from one method of config to the other?

Yes - let's say I set up via configuration.yaml first. To remove it, I would need to delete the entry from the Configuration > Integrations menu, then remove it from my configuration.yaml. At that point it can be re-added, either via the UI or via YAML

[craigbarratt]

Thanks for the explanation.

I'm planning on adding a new boolean config parameter, probably called hass_global, which exposes hass as a global variable. Default is False. I think I can do this correctly by mimicking the code for allow_all_imports. It would be good to get you to review my changes; perhaps I'll submit it as a PR.

Anyhow, I did notice a couple of things looking at the code:

• PYSCRIPT_SCHEMA is duplicated between config_flow.py and __init__.py. There should be one definition and the other should import it from there.
• the strings.json file is identical to translations/en.py. Are both necessary?

[raman325]

Happy to review the changes you plan to make.

re: PYSCRIPT_SCHEMA that's expected. With the next HA release, the original PYSCRIPT_SCHEMA can be imported (we need this to be released: home-assistant/core#41532)
re: translations, only translations/en.py is explicitly required, I included both due to convention, since in core HA generates translations from the original strings.json

[raman325]

I accepted your invite and looked through the code. It makes sense to me, the only thing is in your schema's in config_flow.py you should use bool as the value type rather than cv.boolean. cv.boolean will only work in the next HA release and would be broken for earlier versions of HA

[craigbarratt]

@raman325 - I found a bug related to reload that we need to fix.

There are now two types of "reload" - calling the pyscript.reload service, or using the Configuration -> Integrations -> Pyscript -> reload (from menu). The latter does reload the config, but it causes a couple of problems with pyscript:

• it doesn't unload or cleanup existing pyscript scripts; it simply calls load_scripts() as though it's a fresh start
• it calls hass.bus.async_listen(EVENT_HOMEASSISTANT_STOP, stop_triggers) every time you select reload from the UI. That causes stop_triggers() to get called multiple times on HASS exit or restart, which breaks things.

I assume UI reload (Configuration -> Integrations -> Pyscript -> reload) should work the same as the pyscript.reload service, or is that menu option truly meant to mean just reload the config? I presume that instead of just calling load_scripts(), it should call reload_scripts_handler(), although I presume it doesn't need to reload the yaml config in the startup case; the first part of that function should be skipped.

Can an integration provide a function that is truly only called once at startup? If so, then the hass.bus.async_listen() calls could go there. Otherwise we'd need some global flag that ensures we only register those listeners once.

[raman325]

Good catch - when you reload through the Integrations UI, async_unload_entry (https://github.com/custom-components/pyscript/blob/master/custom_components/pyscript/__init__.py#L211) gets called then async_load_entry (https://github.com/custom-components/pyscript/blob/master/custom_components/pyscript/__init__.py#L74) gets called - there is no unload logic to clean things up and async_load_entry assumes it is always being called only once. There are two options to fix this:

1. The best way would be to add unload logic to async_unload_entry that properly destroys everything loaded/created. This will also make a future uninstallation possible without having to restart HA.
2. In lieu of that, we would need to have logic in async_setup_entry that calls the reload handler if the entry was already previously set up.

I like option 1 better for cleanliness but I don't know what actions need to be taken. Is this enough information for you to proceed with a fix, or would you like me to investigate?

[raman325]

I investigated. What I've done is moved the logic to delete all items from the GlobalContextMgr into its own function so that I can call it on unload. I also added a list of all event subscription callbacks to unsubscribe so that I can unsub from them during unload. I don't know your code well enough to know if this achieves solution 1 completely, but that's what I attempted to do.

Take a look at the changes: #53 - I am unclear on the best way to test this so I could use some help there assuming the logic makes sense