Fix yaml hashtag handling from config panel's custom getter

[oleole39]

The problem

Cf. Point 1 there: YunoHost/issues#2501

Solution

Add quotes to the getter ouput so that read_yaml does not read # as a comment sign.

PR Status

Tested successfully with fontcompare_ynh.
I also tried to run yunohost app config get $app --full --debug with a few other apps (including borg_ynh which has some custom getters defined, and YAML output looked clean for each of them (additional quotes did not seem to break anything).

How to test

Before the fix:

• install fontcompare_ynh (not yet catalog, waiting for this issue to be solved): https://github.com/YunoHost-Apps/fontcompare_ynh
• go to app config panel in webadmin
• pick different colors than default's and save
• refresh page, you will notice that the color you chose have disappeared from config panel (although they have been correctly applied to the app)
• running yunohost app config get fontcompare --full --debug will show None as values for background_color and font_color.

After the fix:

• running yunohost app config get fontcompare --full --debug will show #{HTML_COLOR_CODE} as values for background_color and font_color.

[alexAubin]

Eeeeh I'm not sure that's an appropriate fix though, because the point is that custom getters are supposed to be able to return yaml, for example as in : https://github.com/YunoHost-Apps/vpnclient_ynh/blob/master/scripts/config#L49

So wrapping the result in quote is gonna makes this case explode

As far as I understand, the edge case for hex color code is sort of a "feature" because, well, it starts with # and # means it's a comment in yaml ...

To me the fix is that the custom getter should return the color code wrapped in quote to disambiguate from it being a comment

[oleole39]

So wrapping the result in quote is gonna makes this case explode

OK, I hadn't that case in mind.

the fix is that the custom getter should return the color code wrapped in quote to disambiguate from it being a comment

That's what I've done for now and I don't mind so much if it is becomes permanent, although it feels a bit of a hack.
I will a make mention of it in the config panel documentation.

custom getters are supposed to be able to return yaml

Maybe the issue is that point, i.e. that in practice getters return either litteral yaml (as in the example you give), or values that are kind of turned into yaml via the Popen constructor used in call_async_output() (I understand how the constructor triggers the getters, but not actually how they get associated to their respective key name):

In the case of fontcompare_ynh, raw_content contains:

background:
  #78b7de
font:
  #04ff00

before going through read_yaml() which returns:

{'background': None, 'font': None}

Another idea for a general fix would be to use a regex to identify values in raw_content and put double quotes around them. Could that work when getters return cat << EOF as in the case you mentioned, noting that any potential comment would nevertheless fail to be interpreted as such?

[zamentur]

To me the fix is that the custom getter should return the color code wrapped in quote to disambiguate from it being a comment
What if someone creates a custom getter to return string user value that could contain those chars. For example a password ?
So yes we should promote something like

value="#FFFFFF"
echo "value: \"$value\""

We could create a helper to help packager ?

value="#FFFFFF"
return_value "$value"

Note that # is a problem but : could ba a problem too.

However, I am not sure it's a good idea to add double quote in the value, cause this value could be used by other part of the code of the script config (like validators or setter...). And thinking this, i think i discover a bug with yaml complex data structure in getter: https://github.com/YunoHost/yunohost/blob/dev/helpers/helpers.v2.1.d/config#L216 : the settings seems to be considered as changed everytime /o... BUT it's an other bug.

An other way could be to declare properly if the return is a composite yaml or a single value.

For example, getoption__XXXX() (yaml able to edit all part of the question) and get__XXXX() (simple value, that could also be a yaml value...)? But retro compatibility is needed, => helpers2.2 ???