Fix access to config data now that we are using config flow

[raman325]

This should resolve the issue we are discussing in #31. Since you said you can reproduce the issue @craigbarratt, can you test this and make sure it works? I would also test updating the config and doing a reload to make sure that the logic I changed works.

[raman325]

I don't think the test failure is related to this change but please tell me if that's not true.

Just to review the changes I have made since you mentioned that you don't know much about config flows - I have done a couple of things:

1. On HA load, the configuration.yaml gets loaded and then each integration runs through async_setup, which in pyscripts case initiates a config flow with the context being {"source": "import"} to let the config flow handler know that this is coming from configuration.yaml. The logic for import is to check for an existing entry (means this was already setup) and then updates it if any of the specified parameters (allow_all_apps and now apps) change. I was previously only checking for allow_all_apps because I wasn't aware of apps but I've added support for it in this PR.
2. In all places I can find where the logic was expecting the config to be {DOMAIN: {<data>}}, I have updated it to refer to config_entry.data which will just return <data>
3. In the reload logic, after reloading configuration.yaml, if there is a pyscript config and it's config is different from the config entry's config, it calls the same logic as Create Unique Task Decorator #1 which will update the config entry.

[raman325]

I've also made the update logic more generic. As long as CONF_ALL_KEYS contains all the keys that you expect, the logic will work for any key you add to the schema in the future

[raman325]

Alright, I've simplified the update logic so that it supports mixed configs (setting the config entry up through UI but adding apps through configuration.yaml). If the entry was set up through the UI, then it ignores updating allow_all_imports from configuration.yaml since it is optional in the config and may unintentionally be set to false during the update.

[craigbarratt]

Thanks for the PR and the updates.

I cloned raman325:fix_config_flow and I see two issues:

• sometimes on a cold start, pyscript setup fails to complete, with a warning

2020-10-12 11:23:21 WARNING (MainThread) [homeassistant.bootstrap] Waiting on integrations to complete setup: pyscript

If I quit and start again, it comes up fine. This happens if the yaml config was changed before starting. If I put a log message at the start of async_setup_entry it doesn't get printed.

• On startup, pyscript.config uses regular dict; for example with this yaml:

pyscript:
  allow_all_imports: true
  apps:
    app1:
      - val1: 10
      - val2: 21
  apps_list:
    entry1:
      - val1: 100
      - val2: 200

then pyscript.config is set to

{
    "allow_all_imports": True,
    "apps": {"app1": [{"val1": 10}, {"val2": 21}]},
    "apps_list": {"entry1": [{"val1": 100}, {"val2": 200}]},
}

but on reload if the yaml config has changed, pyscript.config uses OrderedDict:

{
    "allow_all_imports": True,
    "apps": OrderedDict(
        [("app1", [OrderedDict([("val1", 10)]), OrderedDict([("val2", 21)])])]
    ),
    "apps_list": OrderedDict(
        [("entry1", [OrderedDict([("val1", 100)]), OrderedDict([("val2", 200)])])]
    ),
}

It would be great if the reload case was the same as the cold start case (eg, regular dict).

[raman325]

It looks like the two issues are related - the setup only fails after the config entry has been updated with OrderedDicts.

When I call await async_hass_config_yaml(hass), it's returning the config with OrderedDict's. I tested this with the last commit before I introduced config flow running the latest dev HA and the same thing occurs, so it's possible this has always been there but you didn't notice, or that something changed, although I couldn't figure out what that would be. Anyway, still investigating

[raman325]

OK the two weren't related. Fixed the timeout issue, but as far as I can tell, the OrderedDict's thing was always how the config was being pulled in. The difference is that with a config entry, on shutdown HA writes the data to disk and then it becomes a normal dictionary. I can get around this by doing a json.loads(json.dumps(config)) which will always return a normal dictionary, but it seems kludgy. Thoughts?

[raman325]

I went ahead and forced it to always use a dictionary. All startup issues should be gone as well. Let me know if you run into anything else

[craigbarratt]

Thanks for converting the OrderedDict on reload. I don't know a better way to do that.

Unfortunately I still see the first problem: pyscript fails to initialize if the yaml config was modified. Can you replicate that?

[raman325]

I was not testing the right scenario to trigger the failure. I found where the call is failing and found that the line is unnecessary. This should be working 100% finally (hopefully)

EDIT: Found one more bug where if the entry was set up via UI, and then the apps config was added to configuration.yaml , pyscript reloaded, and then the apps config was removed and pyscript was reloaded, the apps wouldn't be removed from the config. Resolved in the last commit

EDIT2: Found one more potential issue, but I'm not sure if it's intended or not because it existed before my changes. When calling the reload service, reload_scripts_handler does not update allow_all_imports so that cannot be "hot" changed, that requires a restart. Is that intentional?

[craigbarratt]

Looks good! Thanks for the quick followup and resolution.

[raman325]

Thanks for the extensive testing! It would be good to add some test coverage for these types of scenarios, I'm just not sure the best way to go about doing it, but if I think of one I will submit a new PR

[craigbarratt]

Agreed, but I'm not sure how to mock the yaml configuration. I just added mock-open to tests/requirements_test.txt so I can add tests for module importing; perhaps that can mock the config file too.

Also, I finally figured out how to add pylint to pre-commit and github actions. I just made a couple of very minor pylint fixes to the config flow.

[raman325]

Awesome! Thanks for the tweak and good catch on the tests

[craigbarratt]

I just released 0.31, which includes the config flow. I then tested it with a new installation of HASS 0.117.0.dev0, installed HACS, then installed pyscript 0.31. It doesn't start - it doesn't get as far as creating the <config>/pyscript directory.

Can you look at this please asap?

[craigbarratt]

Oops - I didn't add pyscript: to config/configuration.yaml. After doing that, it starts now. Sorry about the false alarm.

Just confirming that step is still necessary with config flow?

[craigbarratt]

While it does now show up under "Configuration", the pyscript card doesn't have a "Configure" button. And it doesn't show up when you search for it. So as far as I can tell, config flow is not working on a clean install.

[raman325]

@craigbarratt I just tested it and it is working as expected. To be clear, after installation, you still have to go to Config > Integrations, click on the add button in the bottom right, and then select pyscript. From there it should be straight forward to set it up (this is when you don't have pyscript in your configuration.yaml btw.

Regarding configure: That's only available to a subset of integrations. What we can do in the future is make it so people can add apps config parameters through the UI. The UI doesn't support it yet but I have seen requests for it so I think it will in the future

[raman325]

Also one thing to note, now that it's a config entry, to remove the integration you have to both delete the config entry from the UI and delete it from your configuration.yaml if that's how it was configured

[craigbarratt]

On a clean new install of HASS, HACS and pyscript, pyscript is not shown in Configuration -> Integrations -> "+". There is no sign that config flow is working.

[raman325]

Try hard refreshing the front end and then try to add it. I had this problem in the past when I was developing config flows for other integrations

[craigbarratt]

Ok, I opened an incognito chrome window, logged in, and it does now show pyscript in Configuration -> Integrations -> "+".

However, after checking "allow all imports", I get this error:

Aborted
[%key:common::config_flow::abort::single_instance_allowed%]

Is that because I already set up the yaml configuration?

[raman325]

Yes. That should show a more friendly error but I made a mistake. Core integrations can use common strings HA provides, but I guess custom integrations can't use them. I'll submit a PR to fix that

[craigbarratt]

Ok, thanks. I'll try a fresh install and make sure I use a clean browser session.

[craigbarratt]

I tried a clean install (HASS, HACS, pyscript) with a new incognito browser and I have the same problem. After installing pyscript with HACS, and restart HASS, Configuration -> Integrations -> "+" doesn't show pyscript. Pyscript doesn't initialize.

[raman325]

Did you have the incognito browser open in the frontend before you installed pyscript with HACS? The frontend does some pretty heavy caching, it's best to do a hard refresh of the page after installing a new integration

[craigbarratt]

Yes. I just quit the incognito window and started another one. It worked with a new browser session - I can find pyscript, and I do get prompted for "allow all imports". Wow, that's not a great user experience, but this appears to be an issue outside of pyscript.

The pyscript card now shows in Configuration -> Integrations. However, it doesn't have an "options" link like HACS does. How do you change the config setting for "allow all imports" after the first time?

[raman325]

I just tried a clean install myself and found it:

[raman325]

Well, your reload service logic doesn't handle updates to allow_all_imports even through configuration.yaml. I am happy to add that and to add support to the UI if that's desirable.

[raman325]

The only place you configure the allow_all_imports parameter is here: https://github.com/custom-components/pyscript/blob/master/custom_components/pyscript/__init__.py#L68 and the only place where it gets accessed is here: https://github.com/custom-components/pyscript/blob/master/custom_components/pyscript/eval.py#L724

I can add in support to the UI to make this changeable, but that would require changes to the reload service as well. I asked about this in EDIT2 here but since you didn't respond I assumed you wanted to keep it the same #39 (comment)

[craigbarratt]

Ah good point. At least with the yaml setting you can change it and then re-start HASS. But, yes, adding that to config flow and also reload would be excellent.

Sorry, I missed your edit2 earlier.

[raman325]

no worries! I can work on that next. If I get hass.data[DOMAIN][CONF_ALLOW_ALL_IMPORTS] updated, will the integration pick it up? I guess the question is, does AstEval get initialized on every run, or is it a persistent class that gets accessed?

[craigbarratt]

That would be great - no rush. Most AstEval instances will get recreated during reload, although not for Jupyter sessions which persist. I'll change the code so it doesn't save the value when AstEval is created - it can simply check the config value when it executes each import statement.

[raman325]

I'll handle that in my update