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

helpers: Add a --jinja option to ynh_add_config #1851

Merged
merged 3 commits into from
Jun 4, 2024

Conversation

alexAubin
Copy link
Member

The problem

cf YunoHost-Apps/Experimental_helpers#111

Solution

add a --jinja option to ynh_add_config to render the template using jinja. All bash variables are exported in a subshell. The rendering is done using j2cli which is available in debian since bullseye

PR Status

Tested using new unit tests

How to test

Try rendering a template using ynh_add_config --template=foo --destination=bar --jinja

Copy link
Contributor

@Josue-T Josue-T left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed some improvements on doc side.

Tested on one of my apps it look good.

Thanks

@Salamandar
Copy link
Contributor

Actually I got a request, here in pseudocode :

if filename.endswith(".j2"):
    jinja = True

That would help using jinja2 for nginx/systemd/etc without having to add --jinja arguments to all the relevant helpers.

@alexAubin
Copy link
Member Author

That would help using jinja2 for nginx/systemd/etc without having to add --jinja arguments to all the relevant helpers.

Weeeeell yeah but on the other hand we should be careful and think about what we want exactly ... The current templating is okay-ish and I'm not sure there's a huge gain in moving to jinja for the vast majority of cases, and I'm not sure the syntax is gonna be "cleaner". For example for nginx conf, implementing the whole thing about {% if path != "/" %} and replacing __PATH__/ with either $path/ or just / depending of the install-on-root vs install-on-subpath etc is not going to be way more elegant or whatever.

Or to rephrase, I don't see the gain of reworking all the configs of every app. A lot of energy went into homogeneizing things, I don't feel like seeing a bunch of apps switching every conf to jinja2 just for the sake of it and ending up with heterogenous practices with no benefits

To me this PR is about specific app configurations which are too complex to be handled by the "simplified templating" mechanism

@Josue-T
Copy link
Contributor

Josue-T commented May 28, 2024

From my point of view I'm to migrate progressively everything to jinja for theses raison:

  • Migrating to jinja for an app is quite easy. We can easily update a template with a regex like sed. So, yes the packager need to migrate but it's quite easier than to migrate a package to packaging v2 as most of change can be automated.
  • jinja is a standard template system and is known by many people while the current system is completely custom.
  • To me the magic thing of __PATH__ / which remove the ending / depending of if the app is installed in root is not ideal. Without reading the code we can't really understand what is happening and it not documented at my knowledge. To my point of view for this ideally we should improve this part by reworking the path management.
  • In long temp (by example in packaging v3), we could have also have jinja into the manifest and so by this way this could bring some nice feature to have some calculated value by jinja as same as in ansible. So this could be a way to fix ynh_permission_* helper should not be depreciated package_linter#128. And this way we have standard system unified based on jinja which is great.

Note that this is only my personal point of view. I don't mind if we keep it as it is. I just share my point of view.

@Josue-T
Copy link
Contributor

Josue-T commented May 30, 2024

Hello,

Well, in any case I'm to merge this PR as it is. It still be already a good improvement. The discussion about this can be later, more on packaging v3 side.

@alexAubin alexAubin merged commit fef411e into dev Jun 4, 2024
2 of 3 checks passed
@alexAubin alexAubin deleted the add-jinja-support-to-ynh-add-config branch June 4, 2024 13:02
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.

4 participants