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

Fix yaml hashtag handling from config panel's custom getter #2016

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

oleole39
Copy link
Contributor

@oleole39 oleole39 commented Dec 16, 2024

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
Copy link
Member

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
Copy link
Contributor Author

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?

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

Successfully merging this pull request may close these issues.

2 participants