Adding id to lovelace cards in ui-lovelace.yaml by bramkragten · Pull Request #17498 · home-assistant/core

bramkragten · 2018-10-15T20:50:33Z

Description:

First step to configurate lovelace from the ui, adding ID's to cards in ui-lovelace.yaml

Related issue (if applicable): fixes #

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#<home-assistant.io PR number goes here>

Example entry for `configuration.yaml` (if applicable):

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated in home-assistant.io

If the code communicates with devices, web services, or third-party tools:

New dependencies have been added to the REQUIREMENTS variable (example).
New dependencies are only imported inside functions that use them (example).
New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
New files were added to .coveragerc.

If the code does not interact with devices:

Tests have been added to verify that the new code works.

balloob · 2018-10-15T20:56:24Z

homeassistant/components/lovelace/__init__.py

+    # Check if all cards have an ID or else add one
+    updated = False
+    views = config.get('views')
+    if views:


for view in config.get('views', []):

balloob

Looks good! Ok to merge when tests added. Make sure to stub out the actual operations that do I/O.

balloob · 2018-10-16T07:39:52Z

homeassistant/components/lovelace/__init__.py

+    yaml.indent(sequence=4, offset=2)
+    try:
+        with open(fname, "w", encoding='utf-8') as conf_file:
+            yaml.dump(data, conf_file)


This is dangerous, dump the yaml first to a string, then open file and truncate. Otherwise, if the dumping fails, the lovelace config is wiped out.

Like this?

output = StringIO() yaml.dump(data, output) with open("conf.yaml", 'w', encoding='utf-8') as conf_file: output.seek(0) copyfileobj(output, conf_file) output.close()

So I was googling it, and that's pretty inefficient according to the author of ruamel.yaml.

Why not do this

https://github.com/home-assistant/home-assistant/blob/9c52a3ce223c06f2d29b0e61c59d660dd180e5b8/homeassistant/util/json.py#L53-L56

balloob · 2018-10-16T07:44:16Z

You should not just update the tests, you should also make sure tests are added for the new functionality.

bramkragten · 2018-10-16T10:08:36Z

You should not just update the tests, you should also make sure tests are added for the new functionality.

Yeah I know, going to write test tonight

houndci-bot · 2018-10-16T19:47:24Z

tests/components/lovelace/test_init.py

+    id: example
+    # Optional background (overwrites the global background).
+    background: radial-gradient(crimson, skyblue)
+    # Each view can have a different theme applied. Theme should be defined in the frontend.


line too long (92 > 79 characters)

houndci-bot · 2018-10-16T19:47:24Z

tests/components/lovelace/test_init.py

+  - url: /local/my-webfont.css
+    type: css
+
+# Optional background for all views. Check https://developer.mozilla.org/en-US/docs/Web/CSS/background for more examples.


line too long (121 > 79 characters)

houndci-bot · 2018-10-16T19:47:24Z

tests/components/lovelace/test_init.py

 from homeassistant.setup import async_setup_component
 from homeassistant.components.websocket_api.const import TYPE_RESULT
+from homeassistant.components.lovelace import (WriteError, load_yaml,
+                                     save_yaml, load_config)


continuation line under-indented for visual indent

houndci-bot · 2018-10-16T19:47:25Z

tests/components/lovelace/test_init.py

 from homeassistant.exceptions import HomeAssistantError
 from homeassistant.setup import async_setup_component
 from homeassistant.components.websocket_api.const import TYPE_RESULT
+from homeassistant.components.lovelace import (WriteError, load_yaml,


'homeassistant.components.lovelace.WriteError' imported but unused

houndci-bot · 2018-10-16T19:47:25Z

tests/components/lovelace/test_init.py

+from tempfile import mkdtemp
 from unittest.mock import patch
+from ruamel.yaml import YAML
+from ruamel.yaml.compat import StringIO


'ruamel.yaml.compat.StringIO' imported but unused

houndci-bot · 2018-10-16T19:47:25Z

tests/components/lovelace/test_init.py

 """Test the Lovelace initialization."""
+import os
+import unittest
+import sys


'sys' imported but unused

thomasloven · 2018-10-16T19:55:25Z

man/man1/nosetests.1

@@ -0,0 +1,581 @@
+.\" Man page generated from reStructuredText.


This file was probably added to the commit by mistake, huh?

Uuuuh yeah...

balloob · 2018-10-16T21:04:16Z

homeassistant/components/lovelace/__init__.py

+    """Error writing the data."""
+
+
+def save_yaml(fname: str, data: JSON_TYPE):


So there is a lot of copy paste going on in these methods. I think that it's fine for now.

A bunch of that will go away once we migrate the util.yaml to ruamel.yaml, as it implements YAML 1.2 and not YAML 1.1.

btw, I can only recommend ruamel.yaml. Recently started porting esphomeyaml to use it and it's rather simple - since esphomeyaml uses almost the same loader/yaml_util code (😄), I can then also port it to home assistant

That would be awesome Otto.

balloob · 2018-10-16T21:09:23Z

homeassistant/components/lovelace/__init__.py

+    yaml.indent(sequence=4, offset=2)
+    tmp_fname = fname + "__TEMP__"
+    try:
+        with open(os.open(tmp_fname, O_WRONLY | O_CREAT | O_TRUNC, 0o600),


Use 0o644, it doesn't have to be private

tests/components/lovelace/test_init.py

houndci-bot · 2018-10-17T05:59:39Z

tests/components/lovelace/test_init.py

+from homeassistant.components.lovelace import (load_yaml,
+                                               save_yaml, load_config)

+from tests.common import patch_yaml_files


'tests.common.patch_yaml_files' imported but unused

houndci-bot · 2018-10-17T05:59:39Z

tests/components/lovelace/test_init.py

+import os
+import unittest
+from tempfile import mkdtemp
+from io import StringIO


'io.StringIO' imported but unused

frenck · 2018-10-26T22:39:40Z

So just wondering... why isn't this documented?
This should be documented somewhere, since the docs still state the id on the views are still optional (but now auto generated).

Also the release blog post might have at least mentioned this.

Any view on this @balloob @bramkragten?

agreenfield1 · 2018-10-27T01:25:53Z

This seems to be causing the file permissions to get reset. I have the file shared over Samba for editing purposes, and every time an ID is added the permissions get set back to read-only for group and other.

bramkragten · 2018-10-27T06:31:08Z

The permissions get set to 644, we create a new file and copy that to prevent your config from beining deleted in case of an error.

bramkragten · 2018-10-27T06:32:54Z

@frenck nothing changed from a configuration point, but I will add some lines that we add ids if you don't specify them.

frenck · 2018-10-27T07:06:08Z

@bramkragten I do not agree, for example, the documentation on the Lovelace UI, include the example, states the ID is optional. Apparently, it isn't.

My main issue: Magic happened. Something touched my files. Fills it with UUID's (which damn sure aren't pretty), file permissions get reset and there is literally NOTHING in the docs... That can't be right...

Some questions that aren't answered now:

"What the hell is that?"
"Why is it added?"
"Must the be unique?"
"Can I change them?"
"I want to remove them?"

So docs are definitely needed.

arsaboo · 2018-10-27T15:11:34Z

File permissions should not change....that is a major inconvenience (and requires users to change behavior).

Other than that, all we need to indicate in the docs is that id will be added to ui-lovelace (for upcoming features, e.g., rearranging UI using drag-and-drop). Users can completely ignore the same.

frenck · 2018-10-27T15:18:29Z

Well, I've learned this is no longer limited to file permissions or changing ids
It actually changes file structure in more places.

For example, YAML has multiple options to list multiline strings. This change beautifully migrates it to something else for you. So that might need docs as well.

    cards:
      - type: markdown
        content: "Starting with Home Assistant 0.72, we're experimenting with a new way of defining your interface. We're calling it the **Lovelace UI**."

Becomes

    cards:
      - id: af5c07ccc4c14d978e25ec7148c20ee6  # Automatically created id
        type: markdown
        content: Starting with Home Assistant 0.72, we're experimenting with a new
          way of defining your interface. We're calling it the **Lovelace UI**.

(note the change in content)

arsaboo · 2018-10-27T15:22:27Z

I am guessing it is related to the length and actually improves readability :)

Sure, we should include that in the docs so that users are not surprised.

frenck · 2018-10-27T15:24:14Z

@arsaboo It changes around unrelated stuff, which should not be happening.
Secondly, I like to adhere to my own styling, thank you.

gadgetchnnel · 2018-10-27T15:31:29Z

The permissions get set to 644, we create a new file and copy that to prevent your config from beining deleted in case of an error.

If this is being done, the permissions should be changed back afterwards, otherwise it is a real hassle if we want to be able to update the file over SFTP or Samba.

ludeeus · 2018-10-27T16:46:30Z

When I add something to my configuration I Should be the only one to change it.
I put it there the way I did for a reason...

bramkragten · 2018-10-27T17:11:27Z

If you add an ID to all your cards and views we will not touch your file.
(as long you don't use the edit functions in the UI)

ludeeus · 2018-10-27T17:20:50Z

You clearly did not see this comment in this issue...
#17498 (comment)

bramkragten · 2018-10-27T17:24:28Z

Yes I have. If all cards and views have IDs, we will not add them and won't save the YAML thus not changing the layout.

bramkragten · 2018-10-27T17:33:43Z

It simply is not possible to add things programmatically to YAML without ever changing the layout. The indents are hardcoded as well. So if someone would have a different indentation, what would be perfect YAML, we would still save it with 4 spaces.

So if you care about your styling, and don't want to use the edit functionality, make sure everything has an ID.

frenck · 2018-10-27T17:46:34Z

IMHO we should revert this PR.
Don't just go and fool around with files of people. There is nothing wrong with a proper error message and a good piece of docs guiding the user through an upgrade process. We are used to this, it is called a breaking change and is listed in the release notes as well (where this should have been in btw...)

I don't get why we are having migration/helper logic in this case, while we are usually just marking breaking changes.

If all cards and views have IDs, we will not add them and won't save the YAML thus not changing the layout.

That statement should have been made before and the PR should have been declined in the first place. Just based on that.

So if you care about your styling, and don't want to use the edit functionality, make sure everything has an ID.

So wow, really? That is like saying... If you forget it once... We'll just go ahead and fool around? As a punishment for forgetting things?

I'm sorry, I want things to move forward. But this is just the wrong approach.

dshokouhi · 2018-10-27T17:54:12Z

I think maybe we should treat this upcoming UI editor more like the existing Automation and Script editors. Power users simply don't use these tools for the reasons stated by @bramkragten. We should only touch the lovelace file IF a user decides to SAVE from the UI editor. This way power users can just avoid it (like we do today) and users who like the editors can use it. In fact in order to use the automation editor the user needs to meet certain requirements so we should do the same requirements here.

gadgetchnnel · 2018-10-27T18:05:52Z

I'm only speaking for myself here, but my issue with this isn't so much the fact that the file is being updated with these randomly generated ids (I could probably live with that) but the fact that, in doing this, it is resetting the file permissions on the file which makes it a lot harder to edit the file over SFTP (and possibly Samba, I don't use hass.io so I don't know about that).

OttoWinter · 2018-10-27T18:18:31Z

@frenck I do understand that it's annoying for power users. For them, introducing a breaking change would probably be more useful.

However, once a UI editor comes in we'll have to have a migration system in place. If you're a new user, you should in the future never have to touch YAML files. But every so often the config schema will need to be changed. Then, we can't simply create breaking changes anymore as that's really bad for new users.

I mean we're already using ruamel.yaml for dumping the lovelace config (I think?) - this library stores some info about the input YAML file and restores that when re-writing the file. So in theory things like comments should be preserved automatically (haven't checked). There are some flags to modify how much is preserved, maybe we can fine-tune that.

If we're going to need to use a migration system in the future (for the UI editor), why not integrate the migration system now?

The other option would be to have two different lovelace config systems:

one that's internal and auto-migrates (from a JSON file?)
one that's YAML-based, has breaking changes and is for power-users

The problem with that approach would be that:

Two different systems to do the same thing is not great to maintain
If you want to use the UI editor for most things, but need to use YAML for a small power-user feature, you will have to ditch the entire UI editor.

I think we should focus on making the YAML migration tool better, so that it preserves more of the base format. But constantly having breaking changes is something that I hope lovelace/config entries would solve going forward.

frenck · 2018-10-27T18:20:44Z

@OttoWinter That is still no justification. But thanks for your input.

However, once a UI editor comes in we'll have to have a migration system in place.

Large part of the HA system is moving that logic OUT of the configuration files managed by the power user. This just re-introduces old mistakes.
Until that time, this is just not right and should be reverted in a minor release IMHO.

I'll go ahead an see if I can create a PR that reverts this (since a couple of PR has been dumped on top of this file already this became a little puzzle).

frenck · 2018-10-27T18:23:50Z

PS: Another one came up... "!secrets" no longer work now. Which is kinda a security breaking change.
I can no longer use tokens in my Lovelace UI (e.g., in iframe URL's), which forces me to take out the configuration from Git. (Because it now uses ruamel).

bramkragten · 2018-10-27T18:25:35Z

@gadgetchnnel made a update for that f6b9e68

balloob · 2018-10-27T20:13:45Z

#17890

bramkragten added 3 commits October 15, 2018 22:40

ID is added to cards without ID in ui-lovelace.yaml when loaded

6e24d9c

Hound

e5b9682

Remove ui-lovelace.yaml

e13a178

bramkragten requested a review from a team as a code owner October 15, 2018 20:50

homeassistant added integration: lovelace cla-signed labels Oct 15, 2018

ghost added the in progress label Oct 15, 2018

balloob reviewed Oct 15, 2018

View reviewed changes

Nicer get

c1f274e

fabaff added the Hacktoberfest label Oct 15, 2018

Update tests

d565a75

balloob reviewed Oct 16, 2018

View reviewed changes

bramkragten added 2 commits October 16, 2018 19:30

If YAML dump fails, config not gone

1c46f3b

Add tests

b8d7986

houndci-bot reviewed Oct 16, 2018

View reviewed changes

Woof!

394c03e

thomasloven reviewed Oct 16, 2018

View reviewed changes

Remove nosetests

1522e5e

balloob reviewed Oct 16, 2018

View reviewed changes

tests/components/lovelace/test_init.py Show resolved Hide resolved

Address comments

1c773b1

houndci-bot reviewed Oct 17, 2018

View reviewed changes

bramkragten added 4 commits October 17, 2018 08:00

Woof...

7b9547d

Delete test.yaml

9ab6b28

update rights to saved file

820d65e

fix

e29bab2

bramkragten deleted the lovelace-id branch October 17, 2018 14:37

balloob mentioned this pull request Oct 26, 2018

0.81 #17809

Merged

frenck added the docs-missing label Oct 26, 2018

arsaboo mentioned this pull request Oct 27, 2018

Add ID information to Lovelace view page. home-assistant/home-assistant.io#7153

Merged

2 tasks

bramkragten mentioned this pull request Oct 27, 2018

Add 0.81 ID Lovelace FAQ home-assistant/home-assistant.io#7152

Merged

2 tasks

frenck mentioned this pull request Oct 27, 2018

Unsupported YAML, you can not use !secret in ui-lovelace.yaml #17869

Closed

home-assistant locked as resolved and limited conversation to collaborators Oct 27, 2018

		@@ -0,0 +1,581 @@
		.\" Man page generated from reStructuredText.

		"""Error writing the data."""


		def save_yaml(fname: str, data: JSON_TYPE):

Uh oh!

Conversation

bramkragten commented Oct 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balloob left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bramkragten Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balloob commented Oct 16, 2018

Uh oh!

bramkragten commented Oct 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frenck commented Oct 26, 2018

Uh oh!

agreenfield1 commented Oct 27, 2018

Uh oh!

bramkragten commented Oct 27, 2018

Uh oh!

bramkragten commented Oct 27, 2018

Uh oh!

frenck commented Oct 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arsaboo commented Oct 27, 2018

Uh oh!

frenck commented Oct 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arsaboo commented Oct 27, 2018

Uh oh!

frenck commented Oct 27, 2018

Uh oh!

gadgetchnnel commented Oct 27, 2018

Uh oh!

ludeeus commented Oct 27, 2018

Uh oh!

bramkragten commented Oct 27, 2018

Uh oh!

bramkragten commented Oct 15, 2018 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):

bramkragten Oct 16, 2018 •

edited

Loading

frenck commented Oct 27, 2018 •

edited

Loading

frenck commented Oct 27, 2018 •

edited

Loading

frenck commented Oct 27, 2018 •

edited

Loading

frenck commented Oct 27, 2018 •

edited

Loading

frenck commented Oct 27, 2018 •

edited

Loading