search

YunoHost / package_linter

Linter for YunoHost applications packages
https://yunohost.org/#/packaging_apps
GNU Affero General Public License v3.0
17 stars 13 forks source link

`ynh_permission_*` helper should not be depreciated #128

Open Josue-T opened 4 months ago

Josue-T commented 4 months ago

On some apps like synapse we must enable conditionally some permission. The declarative way in the manifest.toml is great but is also limited. In some case we need more flexibility it's why we still need theses helper to manage theses cases.

tituspijean commented 4 months ago

Reference: https://github.com/YunoHost-Apps/synapse_ynh/blob/b0850e5e517fbc1f300f974a62709977da7684f5/scripts/install#L425-L429

Though, would we actually need that condition if we were using packaging v2, with a domain question in the manifest instead of a string?

alexAubin commented 4 months ago

On one hand, yes, but on the other hand, the point of declarativeness is that you're supposed to know what state to configure just by reading a static file and not having to run any epic install/update/whatever script. If we allow .1% of apps to do "stuff only in scripts" then this means we can't develop any stuff like yunohost app regen-conf that would regen bunch of configurations from the core without calling any app-specific script ...

So imho this just means that we have a shortcoming in the current packaging format, and we should keep track of it. I guess ultimately this means we should have so if = statements in the declarative format with bash snippets (similar to packages_from_raw_bash for the apt resource) but the exact design should also take into account what are the most common use cases to cover

Josue-T commented 4 months ago

Well,

For synapse it's quite specific because we have the domain which is where we provide the service. And on other side we have the server_name which is independent (which could be same) which could be anything the user want. The only constraint is that the user have access to manage it.

And for this part it could be a domain managed by yunohost but it could also be anything else that the user manage somewhere else. In the case of yunohost manage it, we try to configure the .well-known to make it working without any configuration. But if the domain is not managed by yunohost we just don't want to crash because of the domain for the permission don't exist as it's not mandatory. So it's why there are a if.

Josue-T commented 4 months ago

But yes I agree with the idea of a declarative way of if = but currently it's not implemented so the current workaround is to use something like this which is not really clean.

And as same as for this (https://github.com/YunoHost/issues/issues/2245) we depreciate the db helper but we still need it for some apps like seafile.

Tagadda commented 4 months ago

To ensure that only the user selected as admin can access the app I use ynh_permission_update for code-server_ynh in the install and upgrade scripts. https://github.com/search?q=repo%3AYunoHost-Apps%2Fcode-server_ynh%20permission&type=code I think this behavior is needed for some other apps like Kresus.